feat: add in-platform transcript editor to the videos page - #3215
feat: add in-platform transcript editor to the videos page#3215abhalsod-sonata wants to merge 1 commit into
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @abhalsod-sonata! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Submit a signed contributor agreement (CLA)
If you've signed an agreement in the past, you may need to re-sign. Once you've signed the CLA, please allow 1 business day for it to be processed. 🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
mphilbrick211
moved this from Needs Triage
to Needs Tests Run or CLA Signed
in Contributions
mphilbrick211
added
the
needs test run
|
Hi @abhalsod-sonata! in order to get your CLA check to turn green, please have your manager reach out to oscm@axim.org to have you added to our existing entity agreement. Thank you! |
mphilbrick211
removed
the
needs test run
eb30fd5 to
4b28c54
Compare
CLA check resolved! |
|
@bradenmacdonald |
mphilbrick211
added
needs test run
mphilbrick211
moved this from Needs Tests Run or CLA Signed
to Ready for Review
in Contributions
|
@abhalsod-sonata Thanks, this is cool! I'd definitely prefer to have the new files converted to TypeScript and ideally to Since you've expressed that it's helpful to keep the code in sync with your existing code, could you not apply the same changes to the existing codebase as well, so they stay in sync? |
Course authors can now edit video transcripts directly in Studio: "Edit transcript" in a transcript row's action menu opens an editor with per-cue text editing, SRT validation, debounced auto-save, and click-to-seek playback with active-cue highlighting against the video preview. The video's Info content moves below the preview (InfoTab), the sidebar lists transcripts directly, and the row menu label becomes "Info and transcripts" for videos.
4b28c54 to
1aa5b83
Compare
@bradenmacdonald |
|
@mphilbrick211, I’m unable to add @bradenmacdonald as a reviewer. Could you please add them to this PR? |
All set! @bradenmacdonald could you re-enable the checks to run? |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3215 +/- ##
==========================================
- Coverage 95.92% 95.84% -0.08%
==========================================
Files 1397 1401 +4
Lines 33581 34022 +441
Branches 7947 8045 +98
==========================================
+ Hits 32214 32610 +396
- Misses 1308 1352 +44
- Partials 59 60 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
Thanks! This is really cool. I did find a bunch of issues that we should clean up before merging, but feel free to push back on anything that doesn't make sense or is out of scope.
- It seems odd that you can edit an existing transcript but not create one from scratch. Should it be possible to create a blank transcript and then fill it out?
- When the transcript fails to download, it says "Unable to save transcript" which is a confusing error message. It should have a clear, distinct error message for this case.
- Nit/optional: The formatting of the timestamps uses a comma
,as the decimal point marker, which is incorrect for English-speaking countries and looks strange to me. I think it should use.or be based on the user's locale.
- The description says "Depends on feat: return 200 when a transcript upload replaces an existing language openedx-platform#39038", but I can't see in the diff where that would make a difference or where that return code is used.
- There are a lot more minor issues that Claude pointed out to me, which I haven't bothered to re-state here for now. I suggest you get Claude or another AI to review the PR and work through any important issues it reports and then ping me for a final review.
| launchDeleteConfirmation, | ||
| handleTranscript, | ||
| input, | ||
| onEdit, |
There was a problem hiding this comment.
Set defaults here, not using defaultProps:
| onEdit, | |
| onEdit = () => {}, |
|
|
||
| TranscriptActionMenu.defaultProps = { | ||
| onEdit: () => {}, | ||
| }; |
There was a problem hiding this comment.
This is causing a console warning:
Please don't add any new usages of defaultProps.
| import { TRANSCRIPT_FAILURE_STATUSES } from '../../../videos-page/data/constants'; | ||
|
|
||
| const TranscriptColumn = ({ row }) => { | ||
| const TranscriptColumn = ({ row, handleOpenFileInfo }) => { |
There was a problem hiding this comment.
| const TranscriptColumn = ({ row, handleOpenFileInfo }) => { | |
| const TranscriptColumn = ({ row, handleOpenFileInfo = null }) => { |
| TranscriptColumn.defaultProps = { | ||
| handleOpenFileInfo: null, | ||
| }; |
There was a problem hiding this comment.
| TranscriptColumn.defaultProps = { | |
| handleOpenFileInfo: null, | |
| }; |
Please use JS default prop values instead of defaultProps, as defaultProps causes console warnings and will stop working in React 19.
| handleOpenFileInfo(row.original); | ||
| }} | ||
| > | ||
| {transcriptMessage} |
There was a problem hiding this comment.
This is mostly an existing problem, but:
- In this case,
{transcriptMessage}is not translated and will just say(N) availableregardless of the user's language. - Why is it a link when transcripts > 1 but not a link when transcripts = 0. Shouldn't I be able to click on "0 transcripts" to open the dialog and add a transcript?
- The syntax "(3) available" makes no sense. What are the parentheses ( ) even doing? It's not correct punctuation. Just "3 available" is better. "(3 available)" would also be acceptable.
There should be a message in messages.ts like this:
transcriptCountLabel: {
id: 'course-authoring.videos-page.table.transcriptColumn.message',
defaultMessage: '{numOfTranscripts, plural, one {{numOfTranscripts} transcript} other {{numOfTranscripts} transcripts}}',
description: 'Message with the number of transcripts available',
},and you should pass in values={{ numOfTranscripts, }}.
| <Form.Control | ||
| type="text" | ||
| value={cue.text} |
There was a problem hiding this comment.
Rendering the cue text in <Form.Control type="text"> creates a single-line control, and the browser will strip out any newlines:
But SRT files often have multi-line strings, and there are test cases and other parts of the code here clearly designed to handle that. Try using a textarea with autosizing instead? And make sure there is a test that verifies \n is preserved when entering/editing multi-line cues in the actual UI.
There was a problem hiding this comment.
Also, please add accessible names to these fields (aria-label or similar). Same with the timestamp fields. They currently have no accessible name at all, so screen reader users may not know what field is what.
| Header: 'Transcript', | ||
| accessor: 'transcriptStatus', | ||
| Cell: ({ row }) => TranscriptColumn({ row }), | ||
| Cell: ({ row, handleOpenFileInfo }: any) => TranscriptColumn({ row, handleOpenFileInfo }), |
There was a problem hiding this comment.
Is handleOpenFileInfo ever actually passed here? Claude says it's not, and the any is showing us that fact. Perhaps use infoModalContentUnderPreview instead.
| const { data } = await getAuthenticatedHttpClient() | ||
| .get(`${getApiBaseUrl()}${apiUrl}?edx_video_id=${videoId}&language_code=${language}`); |
There was a problem hiding this comment.
This is exactly the same as fetchTranscriptContent below, so it should call fetchTranscriptContent() instead of duplicating the code.
| key={`transcript-${language}`} | ||
| data-testid={`transcript-${language}`} |
There was a problem hiding this comment.
Nit: I don't think we need key outside of a loop, and it's best to avoid data-testid if possible.
| <MenuItem | ||
| as={Button} | ||
| variant="tertiary" | ||
| key={`transcript-actions-${language}-edit`} |
There was a problem hiding this comment.
I don't think this key is doing anything? Same below.
4 participants
Adds an in-platform transcript editor to the Studio videos page. Course authors can
open "Edit transcript" from a transcript row's action menu and edit the SRT in the
browser instead of the download → edit locally → re-upload round-trip.
What's included:
transcript-editor/module: cue-by-cue text editing with SRT parsing/validation(
srtUtils), invalid-SRT alerting, debounced auto-save, and a video preview withclick-to-seek + active-cue highlighting and auto-scroll.
InfoTabvia a newinfoModalContentUnderPreviewslot onFileTable/InfoModal), and the info sidebarlists transcripts directly - this intentionally supersedes the previous tabbed
sidebar layout for videos.
transcripts" (
FileMenu,MoreInfoColumn); two existing tests that asserted theold label on video fixtures are updated.
/transcript_upload/endpoint; the UI distinguishes replace (200) from create (201) - see dependency
below.
Impacted user roles: Course Authors (primary); Developers (new
transcript-editormodule and theinfoModalContentUnderPreviewextension point onthe generic file table).
Screen recording:
Before.mp4
After.mp4
Supporting information
/transcript_upload/) - please merge that first; without it the editor cannotdistinguish replace from create.
Testing instructions
Automated:
npm ci npx jest src/files-and-videos --coverage=false # 20 suites / 229 tests expected greenManual (tutor dev):
containing it) and run Studio.
npm run devin this repo (port 2001); open a course with videos that havetranscripts → Videos page.
the info modal shows the video preview with Info content below it and
transcripts listed in the sidebar.
reopen to confirm persistence; click cues to seek the preview.
returns 201.
Other information
PRs): rebased onto current master, which required - merging with the new
permissionsgating inFileTable/MoreInfoColumn; porting one-line changes intothe TS-renamed
FileMenu.tsxandmessages.tsfiles; re-applying theinjectIntl → useIntlmigration on top of the reworkedTranscript.jsx; andkeeping upstream's
key={idx}fix inTranscriptTab.CourseVideosTablestill passesactiveTab/setActiveTabinto the sidebar factoryfor compatibility; the new sidebar ignores them. Happy to remove that plumbing here
or in a follow-up if preferred.
seekTo()guards theplay()promise and ignoresAbortError(interrupted play on rapid cue-seek/pause/unmount), logging anything else.
Menu,AlertModal, form controls);no dedicated audit yet - feedback welcome, especially on cue-list keyboard
navigation.
descriptions; existing message ids areunchanged so downstream translations carry over.
Best Practices Checklist
.ts,.tsx).- The new
transcript-editor/module is ported as.jsx/.jsto stayreviewably close to the downstream original. Happy to convert
TranscriptEditor/srtUtilsto TS in this PR if preferred - say the word.propTypesanddefaultPropsin any new or modified code.- Ported/modified components follow the surrounding files' existing
PropTypes style; will drop them as part of the TS conversion above if requested.
src/testUtils.tsx(initializeMocks)- Changes extend the existing
videos-pagesuites, which predateinitializeMocks; kept their established setup for consistency.editor state is component-local.)
- Transcript content is fetched via the page's existing axios helpers
(
videos-page/data/api.js); this page has not been migrated to React Query yet.messagesfiles have adescriptionfor translators.../in import paths; use@srcfor parent-folder imports.Relevant backend PR