Skip to content

TT-7764 Show compressed size estimate in too big message,Calc compressed size estimate based on num channels - #754

Draft
sarahentzel wants to merge 8 commits into
developfrom
TT-7764
Draft

sarahentzel wants to merge 8 commits into
developfrom
TT-7764

Conversation

@sarahentzel

Copy link
Copy Markdown
Collaborator

Show compressed size estimate in too big message

Show compressed size estimate in too big message
@sarahentzel sarahentzel changed the title TT-7764 Calc compressed size estimate based on single channgel TT-7764 Show compressed size estimate in too big message,Calc compressed size estimate based on single channel Oct 12, 2026
@sarahentzel sarahentzel changed the title TT-7764 Show compressed size estimate in too big message,Calc compressed size estimate based on single channel TT-7764 Show compressed size estimate in too big message,Calc compressed size estimate based on num channels Oct 12, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Localization regressions, a stale live channel estimate, misleading rounding, and a failing test expectation remain unresolved.

6 open findings
What changed in this PR

Adds channel-aware compressed-size estimates and clearer oversized-file handling for recordings and uploads.

Changes:

  • Estimates Opus output size using duration and channel count.
  • Blocks oversized encoded or fallback WAV uploads.
  • Displays file sizes in localized limit messages and expands tests.
File Description
web/​src/​utils/​filterFilesBySizeLimit.ts Formats rejected files with sizes.
web/​src/​store/​localization/​reducers.tsx Adds and renames size-related strings.
web/​src/​store/​localization/​model.tsx Updates localization interfaces.
web/​src/​store/​localization/​exported-strings-name.json Points to the regenerated bundle.
web/​src/​components/​Uploader.tsx Uses the renamed upload error string.
web/​src/​components/​Uploader.test.tsx Removes an unused import.
web/​src/​components/​MediaUploadContent.tsx Shows each rejected file’s size.
web/​src/​components/​MediaUpload.test.ts Tests oversized-file formatting.
web/​src/​components/​mediaRecordSize.ts Implements channel-aware estimates.
web/​src/​components/​mediaRecordSize.test.ts Tests size and WAV-channel calculations.
web/​src/​components/​MediaRecord.tsx Integrates estimates and post-encode validation.
web/​src/​components/​MediaRecord.test.tsx Expands recording-size tests.
web/​src/​components/​MediaRecord.load.test.tsx Updates localization mocks.
localization/​TranscriberAdmin-en.xlf Adds and updates localized messages.
localization/​TranscriberAdmin-en-1.2.xliff Mirrors localization source changes.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread web/src/components/MediaRecord.test.tsx
Comment thread localization/TranscriberAdmin-en-1.2.xliff Outdated
Comment thread localization/TranscriberAdmin-en.xlf Outdated
Comment thread web/src/components/MediaRecord.tsx Outdated
Comment thread web/src/components/MediaRecord.tsx Outdated
Comment thread web/src/utils/filterFilesBySizeLimit.ts Outdated
@sarahentzel
sarahentzel marked this pull request as ready for review October 13, 2026 15:38
@gtryus

gtryus commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

I reviewed TT-7764 against origin/develop, which covers your 5 commits. Local master is stale and would have pulled in about 30 unrelated commits, so I didn't use it. I couldn't reach the Jira ticket, so the Spec review treats your commit messages as the spec. I checked the two biggest claims myself (the orphaned translations and the stale test mock) and both are real.

Standards

Breaks the localization rules in docs/ai/localization.md

  1. Ids are out of alphabetical order. The renamed units stayed where the old toobig ids were:
    • mediaTab.fileTooBig sits after size (xliff:5900).
    • mediaUpload.fileTooBig sits after title (xliff:6215).
    • The same wrong order carries through to model.tsx and reducers.tsx.
  2. Two new units point at the wrong source file. Both strings are only used in MediaRecord.tsx, but:
    • passageRecord.compressErrorTooBig (xliff:7001) points to passageRecord.tsx.
    • passageRecord.tooBigAfterCompression (xliff:7064) points to MediaTab.tsx.

These parts follow the rules: the generated model.tsx, reducers.tsx and JSON match what the tool produces, the .xlf mirrors the .xliff, new units have an empty <target/>, components read strings with useSelector, and new files use LF line endings.

Judgement calls

  • Renaming ids loses translations. The es, es3, fr, ha, id, pt, ru, ta and zh .xlf files still use mediaTab.toobig and mediaUpload.toobig, so those languages will show English for these messages.
  • Stale test mock. Uploader.test.tsx:35 still mocks toobig, but Uploader.tsx:496 now reads t.fileTooBig.
  • Duplicated code. * 1000000 appears about 5 times in MediaRecord.tsx and again in filterFilesBySizeLimit.ts. The bytes-to-"x.xxMB" formatting is written twice; a shared mb() helper would cover both.
  • Unclear argument. In estimatedUploadBytes(…, compression, false, channels), the bare false/true doesn't say what it means (MediaRecord.tsx:553-565).
  • Module location. components/mediaRecordSize.ts is plain size math with no React in it, so it fits better in utils/.
  • Leftover comment. "ponytail:" at mediaRecordSize.ts:24 looks like a stray note.
  • Repeated failure handling. Both failure paths in MediaRecord.tsx:647-686 repeat the warn / show message / handleSaveFailed steps.

Spec (commit messages used as the spec)

Implemented, but wrong

  • "show filenames and sizes" (R5): the same id renames orphan the existing translations in 9 locales.
  • "Show compressed size estimate in too big message" (R2): the English text for passageRecord.toobig and toobigwarn changed but their ids didn't. Existing translations only contain {1}, so those languages never show the estimate, and translators won't be told the string changed.
  • "single channel" estimate (R1): turning bytes into seconds assumes 48 kHz (mediaRecordSize.ts:756-759). A loaded 16 kHz WAV is underestimated by about 3x. The check after compression still catches it, so the check before saving is just less strict than it should be.

Missing or partial

  • "file too big after conversion" (R4): the upload is blocked, but the message also goes through handleSaveFailed and then saveCompleted. A parent that shows save errors may show it twice. Also, setTooBig(true) is overwritten the next time the effect runs (around line 630).
  • Test gaps:
    • The Uploader too-big path is effectively untested because of the stale mock.
    • There is no MediaRecord test with a stereo recording.
    • There is no test for the near-limit warning during live recording.

Not asked for

  • Renaming the ids wasn't needed. Changing the English text under the old ids would have satisfied R5.
  • If compression fails and the WAV is over the limit, the WAV is no longer uploaded as a fallback (new compressErrorTooBig message). That's a reasonable change, but it isn't in the commit messages.

Confirmed correct:

  • R1 and R6: the mono and stereo size math is right.
  • R3: stereo WAVs are detected from the file header.
  • R7: every too-big message shows 2 decimals.
  • The {n} placeholders match the code that fills them.
  • Sizes use decimal MB throughout.
  • R8: the lint-staged and husky 4 setup is in place.

Summary: Standards has 2 hard violations and 7 judgement calls; the worst is the XLIFF id order. Spec has 3 wrong implementations, 2 partial items and 2 out-of-scope changes; the worst is that renaming the ids breaks existing translations in 9 locales. The simplest fix for both is to keep the old toobig ids.

@gtryus gtryus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

was ‎web/.lintstagedrc supposed to only be on your computer or was it supposed to be part of this commit?

Comment thread web/package.json
"husky": {
"hooks": {}
"hooks": {
"pre-commit": "lint-staged"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

will this work on my machine? I don't have a lint-staged in my package.json file?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

it's on develop, so you probably have it in yours

@sarahentzel
sarahentzel marked this pull request as draft October 13, 2026 21:08
keep too big message after compression toobig
add safety margin in estimate
use downloaded recordings size
fix non english strings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants