fix/refactor #1706: reuse StepSelector in VideoCall, fix moderated proceed flow, correct remote media status, and localize new UI strings#1707
Conversation
…ator/users with accurate media status
…allback-enabled usage)
github-actions
bot
added
low-complexity
size/L
ui/ux
user-test
|
resolving the checks |
There was a problem hiding this comment.
Pull request overview
This PR refactors the moderated test video call interface to use a reusable StepSelector component, fixes the broken proceed-to-next-step flow, corrects participant media status rendering, and adds comprehensive i18n support for the moderated test UI.
Changes:
- Replaced hardcoded custom stepper HTML in VideoCall with embedded StepSelector component with proper parent-child event bridging
- Implemented correct step progression logic in ModeratedTestView that actually advances through lifecycle stages and tasks
- Fixed participant list to use actual remote media flags (
p?.media?.cameraEnabled/microphoneEnabled) instead of role-based fallback, and improved role resolution for moderator/evaluator/observator/participant
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| src/ux/UserTest/components/StepSelector.vue | Added embedded mode support, isModerator gating, task dropdown integration, and i18n for reusable step navigation component |
| src/ux/UserTest/components/VideoCall.vue | Replaced custom stepper with embedded StepSelector, added step/task event bridges, fixed participant media status rendering, and localized all UI strings |
| src/ux/UserTest/views/ModeratedTestView.vue | Implemented correct proceedToNextStep progression logic with switch-based lifecycle and task-loop handling, added handleStepSelected bridge, localized alert messages |
| src/app/plugins/locales/en.json | Added comprehensive i18n keys for StepSelector, VideoCall, and moderated test alerts |
Comments suppressed due to low confidence (1)
src/ux/UserTest/components/StepSelector.vue:456
- The step items have
cursor: pointerand hover effects applied unconditionally, but non-moderators cannot select steps (the click handler checksisModerator). This creates misleading UI feedback. Consider conditionally applying the cursor and hover styles based onisModeratorprop, or add a disabled state class when!isModeratorto visually indicate that the steps are not interactive for non-moderators.
.step-item {
display: flex;
flex-wrap: wrap;
align-items: center;
padding: 16px;
margin-bottom: 8px;
border-radius: 12px;
cursor: pointer;
transition: all 0.2s ease;
border: 2px solid transparent;
}
.step-item:hover {
background: #f5f5f5;
transform: translateX(4px);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| default: | ||
| return | ||
| } | ||
| globalIndex.value = nextGlobalIndex |
There was a problem hiding this comment.
The local state update globalIndex.value = nextGlobalIndex occurs before the database update. If the database update fails, the local state will be out of sync with the persisted state. Consider moving this assignment after the successful database update or wrapping it in a try-catch block to handle potential errors.
| globalIndex.value = nextGlobalIndex | ||
| const roomRef = dbRef(database, `rooms/${roomId.value}`) | ||
| await update(roomRef, { | ||
| globalIndex: globalIndex.value, | ||
| taskIndex: taskIndex.value, | ||
| globalIndex: nextGlobalIndex, | ||
| taskIndex: nextTaskIndex, |
There was a problem hiding this comment.
Missing taskIndex assignment before database update. While nextTaskIndex is correctly calculated, the local taskIndex.value is not updated to match. This could cause the UI to display outdated task information until the next reactive update occurs. Add taskIndex.value = nextTaskIndex after line 830.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| index + 1 === 5 && | ||
| currentStep === 5 && |
There was a problem hiding this comment.
The task selector visibility is hardcoded to check index + 1 === 5, which assumes the tasks step is always at position 5. This creates a tight coupling between the step order and the component logic. If the step structure changes in the future, this condition will need manual updates. Consider making this more dynamic by either passing a prop that indicates which step should show the task selector (e.g., taskStepNumber prop) or having the parent component control this entirely through the showTaskSelector prop without the additional index + 1 === 5 check.
| index + 1 === 5 && | |
| currentStep === 5 && | |
| currentStep === index + 1 && |
| .task-inline { | ||
| } |
There was a problem hiding this comment.
Empty CSS rule .task-inline with no properties. This should either be removed if unused or have its intended styles added. Empty rules add unnecessary noise to the stylesheet.
| .task-inline { | |
| } |
| margin-top: 12px; | ||
| flex: 0 0 100%; | ||
| width: 100%; | ||
| margin-top: 10px; | ||
| } | ||
|
|
There was a problem hiding this comment.
Duplicate margin-top property in .tasks-dropdown. Line 508 sets margin-top: 12px and line 511 sets margin-top: 10px. The second declaration overrides the first, making line 508 redundant. Remove the duplicate to clean up the CSS.
| margin-top: 12px; | |
| flex: 0 0 100%; | |
| width: 100%; | |
| margin-top: 10px; | |
| } | |
| flex: 0 0 100%; | |
| width: 100%; | |
| margin-top: 10px; | |
| } |
| <div class="step-selector"> | ||
| <!-- Step Selector Button --> | ||
| <v-tooltip location="top"> | ||
| <!-- Step Selector Button (Hide toggle button whenn embedded--> |
There was a problem hiding this comment.
Typo in comment: "whenn" should be "when"
| <!-- Step Selector Button (Hide toggle button whenn embedded--> | |
| <!-- Step Selector Button (Hide toggle button when embedded--> |
| case 4: | ||
| if (taskIndex.value < totalTasks - 1) { | ||
| nextTaskIndex = taskIndex.value + 1 | ||
| } else { | ||
| nextGlobalIndex = 5 | ||
| nextTaskIndex = 0 | ||
| } | ||
| break |
There was a problem hiding this comment.
Edge case: if there are no tasks (totalTasks === 0), the proceed logic at globalIndex 4 will immediately advance to globalIndex 5 since 0 < -1 is false. While this may be acceptable behavior (skip tasks step if no tasks), it should be explicitly validated that the test structure always has at least one task when globalIndex 4 is reached, or the UI should prevent reaching this step if no tasks exist. Consider adding a guard or comment explaining this behavior.
| <v-chip | ||
| v-if="participant.isSelf && !isObservator" | ||
| v-if="!isObservator" | ||
| size="x-small" | ||
| :color="participant.hasCamera ? 'green' : 'red'" | ||
| class="ml-1" | ||
| > | ||
| {{ participant.hasCamera ? 'Cámara' : 'Sin cámara' }} | ||
| {{ | ||
| participant.hasCamera | ||
| ? t('UserTestView.VideoCall.participants.cameraOn') | ||
| : t('UserTestView.VideoCall.participants.cameraOff') | ||
| }} | ||
| </v-chip> |
There was a problem hiding this comment.
The camera and microphone status chips use v-if="!isObservator" which checks if the current viewing user is an observator. This means observators cannot see the camera/microphone status of any participants, including the moderator and regular participants. The condition should check whether the individual participant being displayed is an observator (participant.role !== 'observator'), not whether the current user is an observator. Change the condition to v-if="participant.role !== 'observator'" to properly display media status for non-observator participants while hiding it for observators.
| <v-chip | ||
| v-if="participant.isSelf && !isObservator" | ||
| v-if="!isObservator" | ||
| size="x-small" | ||
| :color="participant.hasMicrophone ? 'green' : 'red'" | ||
| class="ml-1" | ||
| > | ||
| {{ | ||
| participant.hasMicrophone ? 'Micrófono' : 'Sin micrófono' | ||
| participant.hasMicrophone | ||
| ? t('UserTestView.VideoCall.participants.microphoneOn') | ||
| : t('UserTestView.VideoCall.participants.microphoneOff') | ||
| }} | ||
| </v-chip> |
There was a problem hiding this comment.
The microphone status chip uses v-if="!isObservator" which checks if the current viewing user is an observator. This means observators cannot see the microphone status of any participants, including the moderator and regular participants. The condition should check whether the individual participant being displayed is an observator (participant.role !== 'observator'), not whether the current user is an observator. Change the condition to v-if="participant.role !== 'observator'" to properly display media status for non-observator participants while hiding it for observators.
| hasCamera: role !== 'observator' && p?.media?.cameraEnabled, | ||
| hasMicrophone: role !== 'observator' && p?.media?.microphoneEnabled, |
There was a problem hiding this comment.
The remote media status relies on p?.media?.cameraEnabled and p?.media?.microphoneEnabled which may be undefined if a participant hasn't updated their status yet. While helper functions isRemoteCameraEnabled and isRemoteMicrophoneEnabled exist (lines 1344-1355) with proper fallback logic, they are not being used here. Consider using these helper functions or adding explicit fallback handling like p?.media?.cameraEnabled ?? false to avoid displaying incorrect status when media object is undefined.
| hasCamera: role !== 'observator' && p?.media?.cameraEnabled, | |
| hasMicrophone: role !== 'observator' && p?.media?.microphoneEnabled, | |
| hasCamera: role !== 'observator' && isRemoteCameraEnabled(p), | |
| hasMicrophone: role !== 'observator' && isRemoteMicrophoneEnabled(p), |
github-actions
bot
added
high-complexity
size/XL
and removed
size/L
low-complexity
labels
|
1 participant
Fixes #1706
Description
This PR replaces VideoCall’s hardcoded custom stepper HTML with an embedded reusable
StepSelectorcomponent, adds proper parent-bridge handling for step/task selection, fixes participant role/media status rendering, and localizes newly introduced moderated-session UI strings.Proof of Work
Before:
Procced-to-Next-Step.Bug.mp4
Stepper.Panel.and.Participant.list.Behaviour.Before.mp4
After:
Stepper.Panel.and.Participant.list.Behaviour.After.mp4
Problem Analysis
Before Fix — What existed on
develop:goToStep()andgoToSpecificTask()existed, but restart-to-welcome flow was missingproceedToNextStepwrote current values back to room (globalIndex: globalIndex.value,taskIndex: taskIndex.value) without advancingparticipantsListused role-based truthy fallback (role !== 'observator') for remote camera/mic chipsImportant upstream reference (proceed no-op):
src/ux/UserTest/views/ModeratedTestView.vue(develop) aroundproceedToNextStep:globalIndexandtaskIndexSolution
StepSelector.StepSelector -> VideoCall -> ModeratedTestView.proceedToNextStep(including task-loop inside global step 4).en.jsonandt().Key Changes
Commit
1c1d4fdb— Replace custom stepper with StepSelector and task navigationsrc/ux/UserTest/components/StepSelector.vueembedded,isModerator,taskItems,currentTaskIndex,showTaskSelector,taskSelectorDisabledv-if="!embedded")showStepPanel = ref(props.embedded)).step-panel-embeddedstyles for inline/panel embeddingisModeratorfor interactive actions (step clicks, go-to/revisit, proceed, restart, task dropdown)taskSelectedemit@click.stopon dropdown wrapper to avoid row click bubblingsrc/ux/UserTest/components/VideoCall.vue<StepSelector embedded ... />stepsListcomputed (Welcome → Completion)normalizedCurrentTaskIndexfor safe task index display/usehandleStepSelectorStep()bridgeresetToWelcome()goToStep(),goToSpecificTask(),taskDropdownItems,showStepperPanel,toggleStepperPanel()src/ux/UserTest/views/ModeratedTestView.vueproceedToNextStep(switch-based lifecycle + task iteration)handleStepSelected({ globalIndex, taskIndex })to sync local state and room state@step-selectedand@proceed-to-next-stepfrom VideoCallCommit
d30e1a8d— Participant role/media consistencysrc/ux/UserTest/components/VideoCall.vueparticipantsList:role !== 'observator')p?.media?.cameraEnabled,p?.media?.microphoneEnabled)Root cause fixed: remote media chips were not using actual per-user media state; they were effectively “always enabled” for non-observators.
Commit
d9268ef8— Localize moderated StepSelector/VideoCall UIsrc/app/plugins/locales/en.jsonUserTestView.StepSelector.*keys:showSteps,hideSteps,title,currentStep,currentStepProgress,allSteps,stepLabel,taskPlaceholder,revisit,goTo,testComplete,proceedToStep,restartTestUserTestView.VideoCall.*keys:welcome→completion)joinVideoCall,maybeLater)src/ux/UserTest/components/StepSelector.vue+src/ux/UserTest/components/VideoCall.vue+src/ux/UserTest/views/ModeratedTestView.vuet(...)usage for new/updated moderated UI textImpact
isModeratoren.jsonFiles Modified
src/ux/UserTest/components/StepSelector.vuesrc/ux/UserTest/components/VideoCall.vuesrc/ux/UserTest/views/ModeratedTestView.vuesrc/app/plugins/locales/en.jsonTesting
Go Toon different stepsProceed to Next Stepen.jsonFuture (Follow-up, out of scope)
es.json, others).