Change Compare to fit mobile view#235
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to improve the Passage Detail “compare/team check” experience on mobile by introducing mobile-specific UI/components and adjusting existing layouts/conditionals in the Passage Detail route.
Changes:
- Add mobile-specific Team Check reference UI and a mobile audio player implementation.
- Adjust Passage Detail grid rendering to hide the standard player on mobile and switch to the mobile Team Check component.
- Update a few Passage Detail-related components/styles to better fit narrow screens, plus a small dependency bump.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/renderer/src/routes/PassageDetail.tsx | Adds mobile TeamCheck component usage and hides the standard player on mobile; minor layout prop change. |
| src/renderer/src/context/PassageDetailContext.tsx | Adds compareMediaId state to the PassageDetail context. |
| src/renderer/src/components/PassageDetail/mobile/TeamCheckReferenceMobile.tsx | New mobile TeamCheck compare UI; currently contains several functional/compile issues. |
| src/renderer/src/components/PassageDetail/mobile/PassageDetailPlayerMobile.tsx | New mobile player implementation reused by the mobile TeamCheck UI. |
| src/renderer/src/components/PassageDetail/PassageDetailMobileDetail.tsx | Minor layout change (centering stack contents). |
| src/renderer/src/components/PassageDetail/Internalization/SelectMyResource.tsx | Adjusts resource selector styling for mobile width constraints. |
| package.json | Bumps @fingerprintjs/fingerprintjs version. |
| package-lock.json | Updates lockfile to match dependency bump. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const handleResource = (id: string) => { | ||
| const row = rowData.find((r) => r.id === id); | ||
| console.log('handleResource', id, row); | ||
| if (row) { | ||
| removeStoredKeys(); | ||
| saveKey(id); | ||
|
|
||
| const segs = getSegments( | ||
| NamedRegions.ProjectResource, | ||
| row.mediafile.attributes.segments | ||
| ); | ||
| const regions = JSON.parse(segs); | ||
| if (regions.length > 0) { | ||
| const { start, end } = regions[0]; | ||
| setMediaStart(start); | ||
| setMediaEnd(end); | ||
| setMediaSelected(id, start, end); | ||
| return; | ||
| } else { | ||
| setMediaStart(undefined); | ||
| setMediaEnd(undefined); | ||
| } | ||
| } | ||
| setMediaId(id); | ||
| }; |
There was a problem hiding this comment.
handleResource returns early after calling setMediaSelected(...), so mediaId is never set for the common case where regions exist. Because playerMediafile and the blob fetch depend on mediaId, the mobile player won’t update/play the newly selected resource. Set mediaId before returning (or derive the player selection from context state instead of local mediaId).
| playerState={ | ||
| { | ||
| loading, | ||
| pdBusy, | ||
| setPDBusy, | ||
| audioBlob: | ||
| blobState.blobStat === BlobStatus.FETCHED | ||
| ? blobState.blob | ||
| : undefined, | ||
| setupLocate, | ||
| playing, | ||
| setPlaying, | ||
| currentstep, | ||
| currentSegmentIndex, | ||
| setCurrentSegment, | ||
| discussionMarkers, | ||
| handleHighlightDiscussion, | ||
| playerMediafile, | ||
| forceRefresh, | ||
| } as IPlayerState | ||
| } |
There was a problem hiding this comment.
The playerState object is being force-cast to IPlayerState, which can hide real type mismatches (e.g., setupLocate callback signature) and lead to runtime breakage. Align the actual functions/fields to IPlayerState and remove the cast so TypeScript can verify the contract.
| {tool === ToolSlug.TeamCheck && isMobile ? ( | ||
| <TeamCheckReferenceMobile /> | ||
| ) : ( | ||
| <TeamCheckReference /> | ||
| )} |
There was a problem hiding this comment.
This ternary renders <TeamCheckReference /> whenever the condition is false, including when tool !== ToolSlug.TeamCheck. That changes behavior from “only show TeamCheckReference on TeamCheck” to “always show TeamCheckReference on non-mobile/non-TeamCheck tools”. Wrap the entire block in tool === ToolSlug.TeamCheck (or use a nested conditional) so other tools don’t render TeamCheckReference.
| {tool === ToolSlug.TeamCheck && isMobile ? ( | |
| <TeamCheckReferenceMobile /> | |
| ) : ( | |
| <TeamCheckReference /> | |
| )} | |
| {tool === ToolSlug.TeamCheck && | |
| (isMobile ? ( | |
| <TeamCheckReferenceMobile /> | |
| ) : ( | |
| <TeamCheckReference /> | |
| ))} |
| const MobileGrid = styled(Grid)<GridProps>(() => ({ | ||
| m: '10%', | ||
| p: '2px', | ||
| display: 'flex', // ← Add this | ||
| width: '80%', // ← Add this | ||
| alignItems: 'center', // ← Add this | ||
| margin: '0 auto', | ||
| justifyContent: 'center', | ||
| alignContent: 'center', | ||
| })); |
There was a problem hiding this comment.
The styled() callback is returning m and p keys, but those are sx shorthands—not valid CSS properties—so they won’t apply. Use margin/padding (or apply sx where the component is used) to ensure the spacing actually takes effect.
| return ( | ||
| <MobileGrid container direction="column"> | ||
| <MobileGrid> | ||
| <StyledGrid ref={containerRef} id="Ryan2" size={{ xs: 12 }}> | ||
| <PassageDetailChooser width={paneWidth} /> | ||
| {tool !== ToolSlug.KeyTerm && ( | ||
| <PassageDetailPlayer | ||
| width={Math.max(playerWidth)} | ||
| allowZoomAndSpeed={true} | ||
| /> | ||
| )} | ||
| </StyledGrid> | ||
| </MobileGrid> | ||
|
|
||
| <MobileGrid> | ||
| <SelectMyResource onChange={handleResource} inResource={resource} /> | ||
| </MobileGrid> | ||
|
|
||
| <MobileGrid> | ||
| <StyledGrid ref={containerRef} id="Ryan2" size={{ xs: 12 }}> | ||
| <PassageDetailChooser width={paneWidth} /> |
There was a problem hiding this comment.
These two StyledGrid instances share the same ref and the same DOM id (Ryan2). Duplicate IDs are invalid HTML and make DOM queries/aria associations unreliable; and a single ref attached to multiple elements will only point to the last one rendered. Use unique IDs (or remove them) and separate refs (or a single wrapping container) for width measurement.
| const onPullTasks = (remoteId: string) => { | ||
| pullTableList( | ||
| 'mediafile', | ||
| Array(remoteId), | ||
| memory, | ||
| remote, | ||
| backup, | ||
| reporter | ||
| ) | ||
| .then(() => { | ||
| forceRefresh(); | ||
| }) | ||
| .finally(() => { | ||
| setSegmentToWhole(); |
There was a problem hiding this comment.
forceRefresh is optional in IPlayerState, but it’s invoked unconditionally here. Under strict TS this is an error (“possibly undefined”) and at runtime it can throw if a caller doesn’t provide it. Either make forceRefresh required in IPlayerState or guard the calls (e.g., forceRefresh?.()).
| import { addPt } from '../utils/addPt'; | ||
| import DiscussionPanel from '../components/Discussions/DiscussionPanel'; | ||
| import PassageDetailMobileDetail from '../components/PassageDetail/PassageDetailMobileDetail'; | ||
| import TeamCheckReferenceMobile from '../components/PassageDetail/mobile/TeamCheckReferenceMobile'; |
There was a problem hiding this comment.
PassageDetailMobileDetail is imported but never used in this file. This will fail lint/typecheck in many setups and also adds dead code; remove the import or render the component where intended.
| import TeamCheckReferenceMobile from '../components/PassageDetail/mobile/TeamCheckReferenceMobile'; |
| maxWidth: '100%', | ||
| overflow: 'hidden', | ||
| }} | ||
| alignItems="center" |
There was a problem hiding this comment.
alignItems="center" on this Box has no effect unless the Box is a flex container (e.g., display: 'flex'). Either add display: 'flex' to the Box styles or remove alignItems to avoid a misleading prop.
| alignItems="center" |
| console.log('setupLocate called'); | ||
| }; | ||
| const handleHighlightDiscussion = (id: string) => { | ||
| console.log('handleHighlightDiscussion called with', start, end); |
There was a problem hiding this comment.
start and end are referenced here but are not defined in scope, which will cause a TypeScript compile error. Update this log to use available values (e.g., only id), or pass start/end into the handler if needed.
| console.log('handleHighlightDiscussion called with', start, end); | |
| console.log('handleHighlightDiscussion called with id', id); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { PassageDetailPlayerMobile } from './PassageDetailPlayerMobile'; | ||
| import PassageDetailPlayer from '../PassageDetailPlayer'; | ||
| import { BlobStatus, useFetchMediaBlob } from '../../../crud/useFetchMediaBlob'; | ||
| import { IMarker } from 'crud/useWaveSurfer'; |
There was a problem hiding this comment.
IMarker is imported via crud/useWaveSurfer, but this repo’s imports use relative paths (and there are no other absolute crud/* imports). This will fail module resolution at build time. Import it from the correct relative path (e.g. ../../../crud/useWaveSurfer).
| import { IMarker } from 'crud/useWaveSurfer'; | |
| import { IMarker } from '../../../crud/useWaveSurfer'; |
| <PassageDetailChooser width={paneWidth} /> | ||
| {(tool !== ToolSlug.KeyTerm || mediafileId) && ( | ||
| {(tool !== ToolSlug.KeyTerm || mediafileId) && !isMobile && ( | ||
| <PassageDetailPlayer | ||
| width={Math.max(0, paneWidth - 40)} | ||
| allowZoomAndSpeed={true} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
This change hides PassageDetailPlayer for all these tools when isMobile is true, but there isn’t an alternative player rendered for mobile in this codepath (except the TeamCheck mobile component). That likely removes the audio player entirely on mobile for Discuss/Record/etc. Consider keeping the player on mobile, or swapping to a mobile-specific player component rather than gating it off globally.
| //const [paneWidth, setPaneWidth] = useState(0); | ||
| const paneWidth = 100; | ||
| const tool = useStepTool(currentstep).tool; | ||
|
|
There was a problem hiding this comment.
paneWidth is hard-coded to 100, but PassageDetailChooser uses this value to compute its usable width. On mobile this will make the chooser extremely narrow regardless of actual available space (you already measure playerWidth from the container). Use the measured container width (or playerWidth) instead of a fixed constant.
| compareMediaId: undefined as string | undefined, | ||
| setCompareMediaId: (_id: string | undefined) => {}, |
There was a problem hiding this comment.
compareMediaId/setCompareMediaId were added to the PassageDetail context state, but there are currently no consumers anywhere in the renderer. If this is meant to support mobile compare, consider wiring TeamCheckReferenceMobile (and any other compare UI) to this context state; otherwise, remove these fields until they are needed to avoid expanding the context API with unused surface area.
| compareMediaId: undefined as string | undefined, | |
| setCompareMediaId: (_id: string | undefined) => {}, |
No description provided.