Show draft tokens in Studio#5729
Show draft tokens in Studio#5729AlexVelezLl wants to merge 2 commits intolearningequality:unstablefrom
Conversation
rtibblesbot
left a comment
There was a problem hiding this comment.
Solid implementation of draft token display and feature-flag gating of the draft publish option.
CI passing. Screenshots provided and look reasonable.
- suggestion: Emit during
<script setup>in PreviewDraftChannelModal may not close the modal reliably (see inline) - suggestion: "Get draft token" menu item and details panel draft token row are not gated by the
test_dev_featurefeature flag (see inline) - suggestion: No backend tests for the new
get_draft_token()model method or thedraft_tokenannotation - nitpick: Label inconsistency between modal and details panel (see inline)
- praise: Good defensive handling — clearing
draft_tokenon live publish, null-safeget_draft_token(), and logging when the modal opens without a token
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Reviewed the pull request diff checking for:
- Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
- Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
- Architecture: duplicated concerns, minimal interfaces, composition over inheritance
- Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
- Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
- Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
- Checked CI status and linked issue acceptance criteria
- For UI changes: inspected screenshots for layout, visual completeness, and consistency
| 'Attempted to open PreviewDraftChannelModal without a draft token. Closing modal.', | ||
| { channelId: props.channel.id }, | ||
| ); | ||
| handleDismiss(); |
There was a problem hiding this comment.
suggestion: Calling emit('close') during <script setup> execution (i.e., before the component is mounted) may not work reliably — the parent's @close handler might not be wired up yet at this point in the lifecycle. The parent already guards with v-if="showPreviewDraftModal && currentChannel" and the menu item checks currentChannel.draft_token, so this path is unlikely to be hit. But if you want a reliable fallback, consider moving this check to onMounted() or using a watchEffect instead.
There was a problem hiding this comment.
Hmm, it makes sense. This is the second time I've heard about this. I think it broadly makes sense, yes.
| </VListTile> | ||
| </template> | ||
| <VListTile | ||
| v-if="currentChannel && currentChannel.draft_token" |
There was a problem hiding this comment.
suggestion: This menu item is only gated by currentChannel.draft_token being truthy, not by the test_dev_feature feature flag. If user A (with the flag) publishes a draft, and user B (without the flag) opens the same channel, user B would see this menu item and the draft token. Is this intentional? If draft tokens should only be visible to users with the feature flag, this should also check hasFeatureEnabled(FeatureFlagKeys.test_dev_feature). The same applies to the draft token row in StudioDetailsPanel.vue (line 51).
There was a problem hiding this comment.
This is okay, yes. We will only hide the draft option radio button for users who do not have the test_dev_feature feature flag.
| <KModal :title="previewYourDraftTitle$()"> | ||
| <template #default> | ||
| <div> | ||
| <strong>{{ channelTokenLabel$() }}</strong> |
There was a problem hiding this comment.
nitpick: This uses channelTokenLabel$() which renders as "Channel token", but StudioDetailsPanel.vue uses draftTokenLabel$() which renders as "Draft token" for the same concept. Since this modal is specifically about a draft channel token, consider using draftTokenLabel$() here for consistency — or vice versa, pick one label for both contexts.
There was a problem hiding this comment.
Didn't question this because this is how it is on the specs, but it really makes a lot of sense, and will save us 1 string from translation.
| "publishing": False, | ||
| "version": channel.version, | ||
| "primary_token": channel.get_human_token().token, | ||
| "draft_token": None, |
There was a problem hiding this comment.
praise: Good call clearing draft_token to None on live publish — this ensures stale draft tokens don't linger in the frontend after a full publish.
| }); | ||
|
|
||
| const showDraftMode = computed(() => | ||
| hasFeatureEnabled.value(FeatureFlagKeys.test_dev_feature), |
There was a problem hiding this comment.
praise: Clean feature-flag gating of the draft publish mode. The dynamic component :is pattern to switch between KRadioButtonGroup and div is a nice way to preserve the live-publish UI for non-flagged users.
53d5c8f to
617670e
Compare
rtibblesbot
left a comment
There was a problem hiding this comment.
Clean implementation of draft token display and feature-flag gating.
CI passing. Screenshots and video verify the full flow: draft publish with snackbar feedback, "Draft published successfully" with Preview action, draft token modal, draft token in details panel, overflow menu item, and correct cleanup on live publish.
1 prior finding resolved. (defensive onMounted handling)
Prior — UNADDRESSED:
- suggestion: No backend tests for
get_draft_token()or thedraft_tokenannotation (see inline)
No new issues found.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
| draft_version = self.channel_versions.filter(version=None).first() | ||
| if not draft_version: | ||
| return None | ||
| return draft_version.secret_token |
There was a problem hiding this comment.
suggestion (UNADDRESSED from prior review): get_draft_token() and the _annotate_draft_token subquery in channel.py both rely on the version=None convention for identifying draft channel versions. A backend test covering this method (and the annotation returning the correct token) would guard against regressions if the ChannelVersion schema or draft lifecycle changes.
617670e to
3220dbf
Compare
rtibblesbot
left a comment
There was a problem hiding this comment.
Clean implementation of draft token display and feature-flag gating.
CI passing. Screenshots and video provided in PR description.
3 prior finding(s) resolved. (backend tests added for get_draft_token() model method and draft_token API annotation)
No new issues found. The subquery annotation, null-safe token handling in publish_next, draft token clearing on live publish, and isDraftPublishing watcher logic all look correct.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
3220dbf to
597fde6
Compare
Summary
test_dev_featurefeature flag enabled.Draft tokens in ChannelEdit:
Grabacion.de.pantalla.2026-03-02.a.la.s.4.35.06.p.m.mov
Side panel for users without
test_def_featureenabled:References
Reviewer guidance
Closes #5461.
Closes #5629.