feat(mobile-app): ✨build Media Player screen#11
Merged
sondoannam merged 7 commits intomainfrom Apr 4, 2026
Merged
Conversation
sondoannam
changed the title
📝 WalkthroughWalkthroughAdds yt-dlp based YouTube stream resolution and a new backend stream-url endpoint; propagates detected source language through AI pipeline progress events; updates MinIO presign flow to use a public presign client; implements a full incremental mobile player (playback, subtitles, layers, controls, local vault, hooks); assorted i18n, tests, and minor cleanups. Changes
Sequence Diagram(s)sequenceDiagram
participant Mobile as Mobile App
participant Backend as Backend API
participant YtDlp as YtDlp Service
participant YouTube as YouTube
Mobile->>Backend: GET /media/:id/stream-url
activate Backend
Backend->>Backend: Validate media origin == YOUTUBE
Backend->>YtDlp: resolveStreamUrls(originUrl)
activate YtDlp
YtDlp->>YouTube: run yt-dlp --dump-single-json
YouTube-->>YtDlp: JSON metadata (formats, urls, title, duration)
YtDlp->>YtDlp: parse formats, select video/audio URLs
YtDlp-->>Backend: StreamUrlInfo (videoUrl, audioUrl, title, duration, expires)
Backend-->>Mobile: 200 {videoUrl, audioUrl, title, duration, expires}
deactivate Backend
sequenceDiagram
participant Producer as AI Producer
participant Translator as Translator/NMT
participant Pipeline as Async Pipeline
participant Events as Progress Publisher
participant Client as Mobile Client
Producer->>Pipeline: initial chunks → detect source lang (holder[0])
Translator->>Pipeline: translation/refinement → update holder[0]
Pipeline->>Events: _publish_progress(progress, step, eta)
Events->>Client: publish_progress(..., sourceLanguage=holder[0])
Client->>Client: update UI with sourceLanguage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (8)
apps/backend-api/src/modules/minio/minio.service.spec.ts (2)
64-96: Consider adding a guard for clientMocks indexing.The test destructures
clientMocks[1]aspublicClientwithout verifying that exactly two clients were created. If the service implementation changes, this could lead to confusing test failures.💡 Optional: Add assertion for client count
it('generates presigned GET urls with the public client host', async () => { const service = new MinioService(createConfigService()); + expect(clientMocks).toHaveLength(2); const [, publicClient] = clientMocks;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend-api/src/modules/minio/minio.service.spec.ts` around lines 64 - 96, Add a guard assertion before destructuring clientMocks to ensure the second mock exists: check clientMocks length or assert Minio.Client was called the expected number of times (e.g., expect(Minio.Client).toHaveBeenCalledTimes(2) or expect(clientMocks.length).toBeGreaterThanOrEqual(2)) so destructuring const [, publicClient] = clientMocks is safe; keep references to clientMocks, publicClient, Minio.Client and the tested method generatePresignedGetUrl to locate where to add this assertion.
47-57: Minor: Config mock type could be improved.The
configobject is implicitly typed, andconfig[key]access relies on string indexing. Consider adding explicit typing for better IDE support and type safety.💡 Optional: Add explicit type annotation
+ const config: Record<string, string> = { - const config = { MINIO_ENDPOINT: 'localhost', ... };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend-api/src/modules/minio/minio.service.spec.ts` around lines 47 - 57, The config mock in createConfigService uses an implicitly typed config variable which makes config[key] unsafe; add an explicit type for that config object (for example Record<string, string | undefined> or a more specific interface keyed by your real config keys) and update the createConfigService signature to use that typed config so get/getOrThrow access uses the typed index; reference the createConfigService function and ConfigService mock to locate where to apply the explicit typing.apps/ai-engine/src/core/smart_aligner.py (1)
51-54: Static analysis flag: ambiguous quotation mark is intentional.Line 54's apostrophe pattern includes
'(RIGHT SINGLE QUOTATION MARK U+2019). This is intentional for handling typographic/"curly" quotes commonly found in transcribed text. Consider adding a brief comment documenting this choice to suppress future linter warnings.- _apostrophe_pattern = re.compile(r"\s+([''][A-Za-z]+)") + # Note: Pattern includes both ASCII apostrophe (') and typographic apostrophe (') + _apostrophe_pattern = re.compile(r"\s+([''][A-Za-z]+)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ai-engine/src/core/smart_aligner.py` around lines 51 - 54, Add a brief inline comment above the _apostrophe_pattern definition in smart_aligner.py explaining that the regex intentionally includes both the ASCII apostrophe (') and the RIGHT SINGLE QUOTATION MARK (U+2019) to correctly handle typographic/“curly” quotes in transcribed text and to suppress future linter warnings about ambiguous quotation marks; mention the variable name _apostrophe_pattern and the regex pattern so reviewers can locate it easily.apps/mobile-app/package.json (1)
63-63: Consider consistent version specifiers.
react-native-webviewuses an exact version (13.15.0) without a prefix, while other Expo packages use~(tilde). For consistency and to receive patch updates, consider using~13.15.0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile-app/package.json` at line 63, Update the dependency specifier for react-native-webview in package.json to use a tilde-prefixed version (change "react-native-webview": "13.15.0" to "react-native-webview": "~13.15.0") so it matches the tilde-style used by other Expo packages; edit the dependencies block in apps/mobile-app/package.json and run a lockfile update/install to persist the change.apps/mobile-app/src/app/(app)/player.tsx (2)
388-395: Remove or enable commented-out code.The
SourceActionscomponent is imported but commented out. If this is intentional for a future iteration, consider removing it entirely and re-adding when ready, or add a TODO comment explaining why it's disabled.♻️ Option 1: Remove completely
- {/* {mediaItem ? ( - <SourceActions - mediaItem={mediaItem} - source={playbackSource.source} - onOpenLayers={() => setLayersVisible(true)} - onOpenYoutube={handleOpenYoutube} - /> - ) : null} */}Also remove the unused import:
import { IconButton, LayerToggle, MediaPane, PlayerControls, SubtitleRow, - // SourceActions, } from "@/components";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile-app/src/app/`(app)/player.tsx around lines 388 - 395, The commented-out JSX using SourceActions (referencing SourceActions, mediaItem, playbackSource.source, setLayersVisible, handleOpenYoutube) is dead code; either restore it or remove it and the unused import. If you intend to keep it disabled, delete the commented block and the SourceActions import, or add a clear TODO comment above the JSX explaining when/why it will be re-enabled and keep the import/use consistent (ensure props mediaItem, playbackSource, setLayersVisible, handleOpenYoutube remain valid if re-enabled).
130-132: Potential unnecessary re-renders fromplaybackobject in dependency array.Including the entire
playbackobject in the dependency array may cause this effect to re-run more often than needed. Consider using onlyplayback.setRateif it has a stable reference.♻️ Proposed fix
useEffect(() => { playback.setRate(playbackSpeed); - }, [playback, playbackSpeed]); + }, [playback.setRate, playbackSpeed]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile-app/src/app/`(app)/player.tsx around lines 130 - 132, The effect in player.tsx currently depends on the whole playback object causing extra runs; change the dependency to the stable method reference (playback.setRate) or a memoized wrapper: inside the component capture the setRate function (e.g., const setRate = playback?.setRate) or create a useCallback/useRef wrapper around playback.setRate, then update the useEffect to depend on setRate and playbackSpeed and call setRate(playbackSpeed); ensure you handle a possibly undefined setRate before calling.apps/mobile-app/src/stores/player.store.ts (1)
80-92: Consider simplifying thetoggleLayerfunction.The current implementation works correctly but can be more concise using a computed property name.
♻️ Proposed simplification
toggleLayer: (layer) => { - if (layer === "phonetic") { - set((state) => ({ showPhonetic: !state.showPhonetic })); - return; - } - - if (layer === "translation") { - set((state) => ({ showTranslation: !state.showTranslation })); - return; - } - - set((state) => ({ showKaraoke: !state.showKaraoke })); + const key = `show${layer.charAt(0).toUpperCase()}${layer.slice(1)}` as + | "showPhonetic" + | "showTranslation" + | "showKaraoke"; + set((state) => ({ [key]: !state[key] })); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile-app/src/stores/player.store.ts` around lines 80 - 92, Simplify toggleLayer by mapping the incoming layer string to the corresponding state key (showPhonetic, showTranslation, showKaraoke) and then call set once with a computed property name that toggles that key; locate the toggleLayer function and replace the three if blocks with a mapping (or conditional) to derive the key and a single set(state => ({ [key]: !state[key] })) call so the logic is concise and avoids multiple returns.apps/mobile-app/src/components/player/SourceActions.tsx (1)
75-104: Extract duplicated YouTube availability check into a variable.The condition
mediaItem.originType === "YOUTUBE" && mediaItem.originUrlis repeated three times. Extract it for clarity and maintainability.♻️ Proposed refactor
export function SourceActions({ mediaItem, source, onOpenLayers, onOpenYoutube, }: SourceActionsProps) { const { theme } = useUnistyles(); const { t } = useTranslation("player"); + + const canOpenYoutube = + mediaItem.originType === "YOUTUBE" && Boolean(mediaItem.originUrl); // ... statusText logic ... <Pressable onPress={onOpenYoutube} - disabled={ - !(mediaItem.originType === "YOUTUBE" && mediaItem.originUrl) - } + disabled={!canOpenYoutube} style={styles.utilityButton} > <Ionicons name="open-outline" size={24} - color={ - mediaItem.originType === "YOUTUBE" && mediaItem.originUrl - ? theme.colors.textSecondary - : theme.colors.disabledText - } + color={canOpenYoutube ? theme.colors.textSecondary : theme.colors.disabledText} /> <Text style={[ styles.utilityLabel, { - color: - mediaItem.originType === "YOUTUBE" && mediaItem.originUrl - ? theme.colors.textSecondary - : theme.colors.disabledText, + color: canOpenYoutube ? theme.colors.textSecondary : theme.colors.disabledText, }, ]} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile-app/src/components/player/SourceActions.tsx` around lines 75 - 104, In SourceActions.tsx, avoid repeating the YouTube availability check by creating a local boolean (e.g., isYoutubeAvailable) computed from mediaItem.originType === "YOUTUBE" && mediaItem.originUrl, then use that variable in the Pressable disabled prop, the Ionicons color expression, and the Text style color expression; update references around onOpenYoutube and keep the existing behavior when the flag is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend-api/src/modules/media/dto/response.dto.ts`:
- Around line 334-383: StreamUrlResponseDto lacks class-validator annotations;
add appropriate validators and imports and apply them to each property: on
videoUrl and thumbnailUrl use `@IsOptional`() + `@IsString`() + `@IsUrl`() (nullable),
on audioUrl, title and originUrl use `@IsString`() and `@IsUrl`() for URLs
(audioUrl/originUrl) or just `@IsString`() for title, on durationSeconds and
expiresInSeconds use `@IsInt`() and `@Min`(0) (or `@IsNumber`() with options) to
enforce non-negative integers; ensure you import decorators from class-validator
and update the StreamUrlResponseDto class definition accordingly (refer to
property names videoUrl, audioUrl, title, durationSeconds, originUrl,
thumbnailUrl, expiresInSeconds).
In `@apps/backend-api/src/modules/media/yt-dlp.service.ts`:
- Around line 67-85: The resolveStreamUrls function currently passes the
incoming youtubeUrl directly to execFileAsync, creating an SSRF risk; before
calling execFileAsync (inside resolveStreamUrls) validate the input using the
URL constructor and enforce an allowlist: only accept protocols 'http:' or
'https:' and hostnames matching 'youtube.com', '*.youtube.com' or 'youtu.be'
(handle 'www.' variants and subdomains via endsWith or exact match), and throw a
controlled error if validation fails so non-YouTube URLs are rejected before
yt-dlp is invoked.
- Around line 73-85: The execFileAsync call that runs yt-dlp (involving
execFileAsync, YT_FORMAT_SELECTOR and youtubeUrl) can exceed Node's default 1MiB
stdout buffer when using --dump-single-json; update the options object passed to
execFileAsync to include an explicit larger maxBuffer (e.g. 10 * 1024 * 1024) so
the child process won't be killed/truncate output and JSON parsing after the
call succeeds.
In `@apps/mobile-app/src/app/`(app)/player.tsx:
- Around line 279-284: Replace the hardcoded eyebrow text in the Player
component with the i18n translation key: add "eyebrow": "KAPTER SUBTITLE PLAYER"
to each locale file under i18n/locales/*/player.json and update the Text in
apps/mobile-app/src/app/(app)/player.tsx to use the translation function (e.g.,
t('player.eyebrow') via useTranslation) instead of the literal string; ensure
useTranslation (or the project's i18n hook) is imported and used in the Player
component so the Text reads the translated value and no hardcoded strings
remain.
In `@apps/mobile-app/src/components/player/MediaPane.tsx`:
- Around line 42-47: Replace hardcoded user-facing strings in the MediaPane
component with i18n keys: update the sourceLabel computation (variable
sourceLabel) to use the translation function (e.g., t('player.device'),
t('player.stream'), t('player.subtitles')) instead of
"DEVICE"/"STREAM"/"SUBTITLES", and replace the other hardcoded captions
("Original video unavailable...", "Subtitle-only mode", "Audio playback", "Video
playback") used in MediaPane (and nearby code around the block flagged at
118-124) with appropriate t('player.*') keys; then add those keys to
apps/mobile-app/src/i18n/locales/*/player.json for each locale so translations
are available.
In `@apps/mobile-app/src/components/player/PlayerControls.tsx`:
- Around line 246-250: The playback speed label is hardcoded with a trailing "x"
— update PlayerControls so the full label is localized via the player namespace
(e.g., use the i18n t function in this component) instead of concatenating
playbackSpeed + "x"; compute the formatted numeric string (from
playbackSpeed.toFixed(2).replace(/\.00$/, "")) and pass it as a parameter to
t('player.playbackSpeed', { speed: formattedSpeed }) so translators can include
the suffix, and replace the current Text children that reference playbackSpeed
with the localized string while keeping the existing styles (styles.sideValue
and theme.colors.textSecondary).
- Around line 36-39: formatTime currently only formats minutes and seconds so
durations >= 3600s show as "61:05"; update the formatTime(seconds: number)
function to detect hours (seconds >= 3600), compute hours = Math.floor(seconds /
3600), then compute remaining minutes and seconds from the remainder, and return
an "H:MM:SS" (or "HH:MM:SS") string with minutes and seconds padded to 2 digits
(and hours as needed) instead of the existing "M:SS" output.
- Around line 198-206: The seek bar (GestureDetector/AnimatedView using
scrubberGesture) is missing required accessibility props for
accessibilityRole="adjustable": add accessibilityValue={{min: 0, max:
durationSec, now: displayedTimeSec, text: formatTime(displayedTimeSec)}}, set
accessibilityActions={[{name: 'increment'}, {name: 'decrement'}]}, and implement
an onAccessibilityAction handler that responds to 'increment' and 'decrement' by
moving the scrubber a small step and updating playback position (call the
existing seek/update position logic used elsewhere so displayedTimeSec and
player position stay in sync). Ensure these props are applied to the same
AnimatedView that currently sets onLayout and accessibilityLabel.
- Around line 262-279: Add "play" and "pause" translation keys to
apps/mobile-app/src/i18n/locales/en/player.json and
apps/mobile-app/src/i18n/locales/vi/player.json (matching existing key style),
then update the Play/Pause Pressable in PlayerControls.tsx to include
accessibilityLabel and accessibilityRole props: compute label via t(isPlaying ?
"play" : "pause") (or the inverse depending on desired wording), and set
accessibilityRole="button"; use the existing onTogglePlayback, isPlaying, and
disabled props to keep current behavior and theme handling.
In `@apps/mobile-app/src/components/player/SubtitleRow.tsx`:
- Line 43: Remove the debug console.log in SubtitleRow.tsx: delete the
console.log("sentence", sentence) call inside the SubtitleRow component (where
the local variable/prop sentence is used) so playback renders no debug output;
ensure no other lingering debug logging remains in the SubtitleRow component or
its render path.
In `@apps/mobile-app/src/hooks/usePlaybackSource.ts`:
- Around line 57-78: The branch handling YOUTUBE split streams incorrectly
downgrades to audio whenever isMergedPlayableVideo(videoUrl, audioUrl) is false,
losing playable video even when videoUrl exists; update the logic inside
usePlaybackSource (the YOUTUBE branch using streamQuery.data and
isMergedPlayableVideo) to return a video-kind source whenever a renderable
videoUrl is available (set kind: "video", uri: videoUrl, sourceKind: "cloud",
canPlayVideo: true, videoUri: videoUrl and keep audioUri if present), and only
fall back to kind: "audio" when no videoUrl is present or is empty/invalid.
Ensure isMergedPlayableVideo is still used to detect merged streams but do not
force audio for split-track results that include a valid videoUrl.
In `@apps/mobile-app/src/hooks/usePlayerSubtitles.ts`:
- Around line 10-15: The hook usePlayerSubtitles is performing a direct fetch of
the subtitle artifact and throwing a hardcoded Error string; move the network
retrieval into the centralized media service (e.g., add a method like
mediaService.fetchSubtitleArtifact or mediaService.downloadSubtitle that accepts
the mediaId/url and returns the response/body) and update the hook to call that
service method instead of fetch(url). Replace the inline throw new Error(`Failed
to fetch subtitles: ${response.status}`) with an app-level error code or
localized message (e.g., use existing localization util or ErrorCodes enum)
returned/raised by the new mediaService method so the hook only handles the
service error rather than hardcoded text.
In `@apps/mobile-app/src/services/local-media-vault.ts`:
- Around line 52-56: The getVault function can throw if JSON.parse encounters
corrupted data; wrap the deserialization in a try/catch inside getVault (after
await ensureVaultDirectory() and after reading AsyncStorage.getItem(VAULT_KEY))
to safely handle malformed JSON: on parse success return the parsed
Record<string, LocalVaultEntry>, on failure log the error, clear or reset the
stored value for VAULT_KEY (e.g., AsyncStorage.removeItem or write an empty
object) and return an empty object so all vault read paths remain recoverable;
reference getVault, ensureVaultDirectory, VAULT_KEY, AsyncStorage, and
LocalVaultEntry when making the change.
- Around line 66-93: The current copyIntoVault always writes a new timestamped
file and overwrites metadata, leaving the old file orphaned; modify
copyIntoVault to (1) after creating targetUri with copyAsync, call
this.saveEntry(entry), (2) if there is an existing entry for the same
mediaItemId (lookup via something like this.getEntry(input.mediaItemId) or
this.findEntryByMediaItemId before copying), store its localUri, and after
successful save unlink the previous file (use fs/promises.unlink or your
project's removeAsync) to avoid orphaned files, and (3) on failure to saveEntry,
delete the newly copied targetUri so you don't create a new orphan—use the
existing symbols copyAsync, saveEntry, VAULT_DIR, sanitizeFileName,
SaveLocalVaultEntryInput, and LocalVaultEntry to locate and implement this
behavior.
In `@apps/mobile-app/src/theme/light.ts`:
- Around line 126-127: Replace the hardcoded hex values assigned to
gradientStart and gradientEnd in the light theme with the centralized design
tokens from the mobile token library (e.g., import and use tokens from
react-native-unistyles or the app's tokens module) so theming follows the mobile
rule; update the import in apps/mobile-app/src/theme/light.ts and set
theme.gradientStart and theme.gradientEnd to the appropriate token names
(referencing the token keys your project uses for gradient start/end colors)
instead of "#F7FBFF" and "#EAF2FB".
---
Nitpick comments:
In `@apps/ai-engine/src/core/smart_aligner.py`:
- Around line 51-54: Add a brief inline comment above the _apostrophe_pattern
definition in smart_aligner.py explaining that the regex intentionally includes
both the ASCII apostrophe (') and the RIGHT SINGLE QUOTATION MARK (U+2019) to
correctly handle typographic/“curly” quotes in transcribed text and to suppress
future linter warnings about ambiguous quotation marks; mention the variable
name _apostrophe_pattern and the regex pattern so reviewers can locate it
easily.
In `@apps/backend-api/src/modules/minio/minio.service.spec.ts`:
- Around line 64-96: Add a guard assertion before destructuring clientMocks to
ensure the second mock exists: check clientMocks length or assert Minio.Client
was called the expected number of times (e.g.,
expect(Minio.Client).toHaveBeenCalledTimes(2) or
expect(clientMocks.length).toBeGreaterThanOrEqual(2)) so destructuring const [,
publicClient] = clientMocks is safe; keep references to clientMocks,
publicClient, Minio.Client and the tested method generatePresignedGetUrl to
locate where to add this assertion.
- Around line 47-57: The config mock in createConfigService uses an implicitly
typed config variable which makes config[key] unsafe; add an explicit type for
that config object (for example Record<string, string | undefined> or a more
specific interface keyed by your real config keys) and update the
createConfigService signature to use that typed config so get/getOrThrow access
uses the typed index; reference the createConfigService function and
ConfigService mock to locate where to apply the explicit typing.
In `@apps/mobile-app/package.json`:
- Line 63: Update the dependency specifier for react-native-webview in
package.json to use a tilde-prefixed version (change "react-native-webview":
"13.15.0" to "react-native-webview": "~13.15.0") so it matches the tilde-style
used by other Expo packages; edit the dependencies block in
apps/mobile-app/package.json and run a lockfile update/install to persist the
change.
In `@apps/mobile-app/src/app/`(app)/player.tsx:
- Around line 388-395: The commented-out JSX using SourceActions (referencing
SourceActions, mediaItem, playbackSource.source, setLayersVisible,
handleOpenYoutube) is dead code; either restore it or remove it and the unused
import. If you intend to keep it disabled, delete the commented block and the
SourceActions import, or add a clear TODO comment above the JSX explaining
when/why it will be re-enabled and keep the import/use consistent (ensure props
mediaItem, playbackSource, setLayersVisible, handleOpenYoutube remain valid if
re-enabled).
- Around line 130-132: The effect in player.tsx currently depends on the whole
playback object causing extra runs; change the dependency to the stable method
reference (playback.setRate) or a memoized wrapper: inside the component capture
the setRate function (e.g., const setRate = playback?.setRate) or create a
useCallback/useRef wrapper around playback.setRate, then update the useEffect to
depend on setRate and playbackSpeed and call setRate(playbackSpeed); ensure you
handle a possibly undefined setRate before calling.
In `@apps/mobile-app/src/components/player/SourceActions.tsx`:
- Around line 75-104: In SourceActions.tsx, avoid repeating the YouTube
availability check by creating a local boolean (e.g., isYoutubeAvailable)
computed from mediaItem.originType === "YOUTUBE" && mediaItem.originUrl, then
use that variable in the Pressable disabled prop, the Ionicons color expression,
and the Text style color expression; update references around onOpenYoutube and
keep the existing behavior when the flag is false.
In `@apps/mobile-app/src/stores/player.store.ts`:
- Around line 80-92: Simplify toggleLayer by mapping the incoming layer string
to the corresponding state key (showPhonetic, showTranslation, showKaraoke) and
then call set once with a computed property name that toggles that key; locate
the toggleLayer function and replace the three if blocks with a mapping (or
conditional) to derive the key and a single set(state => ({ [key]: !state[key]
})) call so the logic is concise and avoids multiple returns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f57c296c-9635-4a8d-8fa2-c1901192a55d
⛔ Files ignored due to path filters (1)
apps/mobile-app/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (46)
.bg-shell/manifest.jsonapps/ai-engine/src/async_pipeline.pyapps/ai-engine/src/core/smart_aligner.pyapps/ai-engine/src/events.pyapps/backend-api/src/modules/media/dto/response.dto.tsapps/backend-api/src/modules/media/media.controller.tsapps/backend-api/src/modules/media/media.module.tsapps/backend-api/src/modules/media/media.service.tsapps/backend-api/src/modules/media/yt-dlp.service.tsapps/backend-api/src/modules/minio/minio.service.spec.tsapps/backend-api/src/modules/minio/minio.service.tsapps/backend-api/src/modules/socket/socket.types.tsapps/mobile-app/app.jsonapps/mobile-app/package.jsonapps/mobile-app/src/app/(app)/_layout.tsxapps/mobile-app/src/app/(app)/index.tsxapps/mobile-app/src/app/(app)/player.tsxapps/mobile-app/src/app/(app)/processing.tsxapps/mobile-app/src/app/_layout.tsxapps/mobile-app/src/components/IconButton.tsxapps/mobile-app/src/components/index.tsapps/mobile-app/src/components/player/LayerToggle.tsxapps/mobile-app/src/components/player/MediaPane.tsxapps/mobile-app/src/components/player/PlayerControls.tsxapps/mobile-app/src/components/player/SourceActions.tsxapps/mobile-app/src/components/player/SubtitleRow.tsxapps/mobile-app/src/constants/endpoint.tsapps/mobile-app/src/entry.tsapps/mobile-app/src/hooks/useActiveSentence.tsapps/mobile-app/src/hooks/useMediaPlayback.tsapps/mobile-app/src/hooks/usePlaybackSource.tsapps/mobile-app/src/hooks/usePlayerSubtitles.tsapps/mobile-app/src/hooks/useProcessingSubtitles.tsapps/mobile-app/src/hooks/useSocketSync.tsapps/mobile-app/src/i18n/i18n.tsapps/mobile-app/src/i18n/locales/en/player.jsonapps/mobile-app/src/i18n/locales/en/processing.jsonapps/mobile-app/src/i18n/locales/vi/player.jsonapps/mobile-app/src/i18n/locales/vi/processing.jsonapps/mobile-app/src/services/local-media-vault.tsapps/mobile-app/src/services/media.services.tsapps/mobile-app/src/stores/player.store.tsapps/mobile-app/src/theme/light.tsapps/mobile-app/src/types/media.tsapps/mobile-app/src/types/socket-events.tsapps/mobile-app/src/utils/subtitle-normalization.ts
💤 Files with no reviewable changes (1)
- .bg-shell/manifest.json
Comment on lines
+334
to
+383
| export class StreamUrlResponseDto { | ||
| @ApiProperty({ | ||
| description: | ||
| 'Direct video stream URL (may be null for audio-only content or when yt-dlp resolves only audio)', | ||
| nullable: true, | ||
| example: | ||
| 'https://rr2---sn-xxx.googlevideo.com/videoplayback?expire=...&itag=248&...', | ||
| }) | ||
| videoUrl: string | null; | ||
|
|
||
| @ApiProperty({ | ||
| description: | ||
| 'Direct audio-only stream URL. Always present. Falls back to the video URL when no separate audio track exists.', | ||
| example: | ||
| 'https://rr2---sn-xxx.googlevideo.com/videoplayback?expire=...&itag=251&...', | ||
| }) | ||
| audioUrl: string; | ||
|
|
||
| @ApiProperty({ | ||
| description: 'Video title from YouTube metadata', | ||
| example: 'Learn English with Friends | Season 1 Episode 1', | ||
| }) | ||
| title: string; | ||
|
|
||
| @ApiProperty({ | ||
| description: 'Duration of the video in seconds', | ||
| example: 1320, | ||
| }) | ||
| durationSeconds: number; | ||
|
|
||
| @ApiProperty({ | ||
| description: 'Original YouTube URL stored in the media record', | ||
| example: 'https://www.youtube.com/watch?v=dQw4w9WgXcQ', | ||
| }) | ||
| originUrl: string; | ||
|
|
||
| @ApiProperty({ | ||
| description: 'YouTube thumbnail URL (null if unavailable)', | ||
| nullable: true, | ||
| example: 'https://i.ytimg.com/vi/dQw4w9WgXcQ/maxresdefault.jpg', | ||
| }) | ||
| thumbnailUrl: string | null; | ||
|
|
||
| @ApiProperty({ | ||
| description: | ||
| 'Approximate seconds until the signed stream URLs expire (~6 hours for YouTube)', | ||
| example: 21600, | ||
| }) | ||
| expiresInSeconds: number; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the new DTO has class-validator decorators/imports.
rg -n -C3 'class StreamUrlResponseDto|class-validator|@Is[A-Za-z]+' apps/backend-api/src/modules/media/dto/response.dto.tsRepository: KMA-Final-Project/project
Length of output: 338
🏁 Script executed:
# Read the full DTO class definition (lines 334-383)
sed -n '334,383p' apps/backend-api/src/modules/media/dto/response.dto.tsRepository: KMA-Final-Project/project
Length of output: 1506
🏁 Script executed:
# Check if class-validator is imported anywhere in the file
rg -n "import.*class-validator|from 'class-validator'" apps/backend-api/src/modules/media/dto/response.dto.tsRepository: KMA-Final-Project/project
Length of output: 51
🏁 Script executed:
# Check other DTOs in the same file to see if they use class-validator
rg -n "@Is[A-Za-z]+" apps/backend-api/src/modules/media/dto/response.dto.ts | head -20Repository: KMA-Final-Project/project
Length of output: 51
Add class-validator decorators to StreamUrlResponseDto.
The new response DTO only has Swagger metadata; repository policy requires DTO validation annotations as well.
Proposed update
+import {
+ IsInt,
+ IsOptional,
+ IsString,
+ IsUrl,
+ Min,
+} from 'class-validator';
export class StreamUrlResponseDto {
@@
+ `@IsOptional`()
+ `@IsUrl`()
videoUrl: string | null;
@@
+ `@IsUrl`()
audioUrl: string;
@@
+ `@IsString`()
title: string;
@@
+ `@IsInt`()
+ `@Min`(0)
durationSeconds: number;
@@
+ `@IsUrl`()
originUrl: string;
@@
+ `@IsOptional`()
+ `@IsUrl`()
thumbnailUrl: string | null;
@@
+ `@IsInt`()
+ `@Min`(1)
expiresInSeconds: number;
}As per coding guidelines: apps/backend-api/src/**/*.dto.ts must use DTO classes with class-validator for request/response validation in NestJS backend.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend-api/src/modules/media/dto/response.dto.ts` around lines 334 -
383, StreamUrlResponseDto lacks class-validator annotations; add appropriate
validators and imports and apply them to each property: on videoUrl and
thumbnailUrl use `@IsOptional`() + `@IsString`() + `@IsUrl`() (nullable), on audioUrl,
title and originUrl use `@IsString`() and `@IsUrl`() for URLs (audioUrl/originUrl)
or just `@IsString`() for title, on durationSeconds and expiresInSeconds use
`@IsInt`() and `@Min`(0) (or `@IsNumber`() with options) to enforce non-negative
integers; ensure you import decorators from class-validator and update the
StreamUrlResponseDto class definition accordingly (refer to property names
videoUrl, audioUrl, title, durationSeconds, originUrl, thumbnailUrl,
expiresInSeconds).
Comment on lines
+67
to
+85
| async resolveStreamUrls(youtubeUrl: string): Promise<StreamUrlInfo> { | ||
| this.logger.log(`Resolving stream URLs for: ${youtubeUrl}`); | ||
|
|
||
| let rawJson: YtDlpJson; | ||
|
|
||
| try { | ||
| const { stdout } = await execFileAsync( | ||
| 'yt-dlp', | ||
| [ | ||
| '--dump-single-json', | ||
| '-f', | ||
| YT_FORMAT_SELECTOR, | ||
| '--no-playlist', | ||
| '--no-warnings', | ||
| '--extractor-args', | ||
| 'youtube:player_client=android', // More reliable for direct URLs | ||
| youtubeUrl, | ||
| ], | ||
| { timeout: 30_000 }, |
There was a problem hiding this comment.
Reject non-YouTube URLs before invoking yt-dlp.
resolveStreamUrls() passes the raw youtubeUrl straight into a network-capable subprocess. If caller validation is ever skipped or bypassed, this service becomes an SSRF primitive instead of a YouTube-only resolver. Validate the scheme/host here and reject anything outside a YouTube allowlist.
🛡️ Suggested guard
async resolveStreamUrls(youtubeUrl: string): Promise<StreamUrlInfo> {
+ let parsedUrl: URL;
+ try {
+ parsedUrl = new URL(youtubeUrl);
+ } catch {
+ throw new BadRequestException('Invalid YouTube URL.');
+ }
+
+ const hostname = parsedUrl.hostname.replace(/^www\./, '');
+ const isYouTubeHost =
+ hostname === 'youtu.be' ||
+ hostname === 'youtube.com' ||
+ hostname.endsWith('.youtube.com');
+
+ if (!isYouTubeHost) {
+ throw new BadRequestException('Only YouTube URLs are supported.');
+ }
+
this.logger.log(`Resolving stream URLs for: ${youtubeUrl}`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async resolveStreamUrls(youtubeUrl: string): Promise<StreamUrlInfo> { | |
| this.logger.log(`Resolving stream URLs for: ${youtubeUrl}`); | |
| let rawJson: YtDlpJson; | |
| try { | |
| const { stdout } = await execFileAsync( | |
| 'yt-dlp', | |
| [ | |
| '--dump-single-json', | |
| '-f', | |
| YT_FORMAT_SELECTOR, | |
| '--no-playlist', | |
| '--no-warnings', | |
| '--extractor-args', | |
| 'youtube:player_client=android', // More reliable for direct URLs | |
| youtubeUrl, | |
| ], | |
| { timeout: 30_000 }, | |
| async resolveStreamUrls(youtubeUrl: string): Promise<StreamUrlInfo> { | |
| let parsedUrl: URL; | |
| try { | |
| parsedUrl = new URL(youtubeUrl); | |
| } catch { | |
| throw new BadRequestException('Invalid YouTube URL.'); | |
| } | |
| const hostname = parsedUrl.hostname.replace(/^www\./, ''); | |
| const isYouTubeHost = | |
| hostname === 'youtu.be' || | |
| hostname === 'youtube.com' || | |
| hostname.endsWith('.youtube.com'); | |
| if (!isYouTubeHost) { | |
| throw new BadRequestException('Only YouTube URLs are supported.'); | |
| } | |
| this.logger.log(`Resolving stream URLs for: ${youtubeUrl}`); | |
| let rawJson: YtDlpJson; | |
| try { | |
| const { stdout } = await execFileAsync( | |
| 'yt-dlp', | |
| [ | |
| '--dump-single-json', | |
| '-f', | |
| YT_FORMAT_SELECTOR, | |
| '--no-playlist', | |
| '--no-warnings', | |
| '--extractor-args', | |
| 'youtube:player_client=android', // More reliable for direct URLs | |
| youtubeUrl, | |
| ], | |
| { timeout: 30_000 }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend-api/src/modules/media/yt-dlp.service.ts` around lines 67 - 85,
The resolveStreamUrls function currently passes the incoming youtubeUrl directly
to execFileAsync, creating an SSRF risk; before calling execFileAsync (inside
resolveStreamUrls) validate the input using the URL constructor and enforce an
allowlist: only accept protocols 'http:' or 'https:' and hostnames matching
'youtube.com', '*.youtube.com' or 'youtu.be' (handle 'www.' variants and
subdomains via endsWith or exact match), and throw a controlled error if
validation fails so non-YouTube URLs are rejected before yt-dlp is invoked.
Comment on lines
+73
to
+85
| const { stdout } = await execFileAsync( | ||
| 'yt-dlp', | ||
| [ | ||
| '--dump-single-json', | ||
| '-f', | ||
| YT_FORMAT_SELECTOR, | ||
| '--no-playlist', | ||
| '--no-warnings', | ||
| '--extractor-args', | ||
| 'youtube:player_client=android', // More reliable for direct URLs | ||
| youtubeUrl, | ||
| ], | ||
| { timeout: 30_000 }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/backend-api/src/modules/media/yt-dlp.service.ts | head -100Repository: KMA-Final-Project/project
Length of output: 3795
🌐 Web query:
Node.js execFile maxBuffer default limit
💡 Result:
The default maxBuffer limit for Node.js child_process.execFile is 1024 * 1024 bytes (1 MB). This applies to both stdout and stderr combined. If exceeded, the child process is terminated, and output is truncated. This has been the default since Node.js v12.0.0 (April 2019), changed from the previous 200 * 1024 bytes (~200 KB) via PR #27179 to address complaints that 200 KB was too small (Issue #9829). Current official documentation for recent versions (v18+, v22+, v25+) confirms "Default: 1024 * 1024". Older versions (v8) used 200 * 1024.
Citations:
- 1: https://nodejs.org/docs/latest/api/child_process.html
- 2: https://nodejs.org/api/child_process.html
- 3: https://nodejs.org/docs/latest-v22.x/api/child_process.html
- 4: child_process: change the defaults maxBuffer size nodejs/node#27179
🌐 Web query:
yt-dlp --dump-single-json output size bytes typical YouTube video
💡 Result:
The output size of yt-dlp --dump-single-json for a typical YouTube video is typically 100-500 KB, but can reach several MB (up to 5+ MB) for live streams or videos with extensive format lists and long URLs.
Citations:
- 1: Selective --dump-json (don't want to download loads of format data) yt-dlp/yt-dlp#5701
- 2: https://gist.github.com/artlung/83c68fdbbdcbbbfcf72db4374a6309da
- 3: [Question] How to extract filesize before download? yt-dlp/yt-dlp#947
- 4: [YouTube] Unexpected high data usage when downloading JSON with yt-dlp over VPN yt-dlp/yt-dlp#12229
Set an explicit maxBuffer for --dump-single-json to handle large metadata.
The execFileAsync call relies on Node.js's default 1 MiB maxBuffer. YouTube metadata from yt-dlp --dump-single-json can exceed this limit (reaching 5+ MB for live streams or videos with many format options), causing the child process to terminate and output to be truncated, which will fail the JSON parse on line 88.
Suggested change
- { timeout: 30_000 },
+ {
+ timeout: 30_000,
+ maxBuffer: 10 * 1024 * 1024,
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const { stdout } = await execFileAsync( | |
| 'yt-dlp', | |
| [ | |
| '--dump-single-json', | |
| '-f', | |
| YT_FORMAT_SELECTOR, | |
| '--no-playlist', | |
| '--no-warnings', | |
| '--extractor-args', | |
| 'youtube:player_client=android', // More reliable for direct URLs | |
| youtubeUrl, | |
| ], | |
| { timeout: 30_000 }, | |
| const { stdout } = await execFileAsync( | |
| 'yt-dlp', | |
| [ | |
| '--dump-single-json', | |
| '-f', | |
| YT_FORMAT_SELECTOR, | |
| '--no-playlist', | |
| '--no-warnings', | |
| '--extractor-args', | |
| 'youtube:player_client=android', // More reliable for direct URLs | |
| youtubeUrl, | |
| ], | |
| { | |
| timeout: 30_000, | |
| maxBuffer: 10 * 1024 * 1024, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/backend-api/src/modules/media/yt-dlp.service.ts` around lines 73 - 85,
The execFileAsync call that runs yt-dlp (involving execFileAsync,
YT_FORMAT_SELECTOR and youtubeUrl) can exceed Node's default 1MiB stdout buffer
when using --dump-single-json; update the options object passed to execFileAsync
to include an explicit larger maxBuffer (e.g. 10 * 1024 * 1024) so the child
process won't be killed/truncate output and JSON parsing after the call
succeeds.
Comment on lines
+279
to
+284
| <Text | ||
| style={[styles.headerEyebrow, { color: theme.colors.primaryLight }]} | ||
| > | ||
| KAPTER SUBTITLE PLAYER | ||
| </Text> | ||
| </View> |
There was a problem hiding this comment.
Hardcoded string violates coding guidelines.
The eyebrow text "KAPTER SUBTITLE PLAYER" should use the i18n translation system instead of being hardcoded.
🔧 Proposed fix
<Text
style={[styles.headerEyebrow, { color: theme.colors.primaryLight }]}
>
- KAPTER SUBTITLE PLAYER
+ {t("eyebrow")}
</Text>Add the corresponding translation key to apps/mobile-app/src/i18n/locales/*/player.json:
{
"eyebrow": "KAPTER SUBTITLE PLAYER"
}As per coding guidelines: "No hardcoded colors or strings in mobile app code".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Text | |
| style={[styles.headerEyebrow, { color: theme.colors.primaryLight }]} | |
| > | |
| KAPTER SUBTITLE PLAYER | |
| </Text> | |
| </View> | |
| <Text | |
| style={[styles.headerEyebrow, { color: theme.colors.primaryLight }]} | |
| > | |
| {t("eyebrow")} | |
| </Text> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/mobile-app/src/app/`(app)/player.tsx around lines 279 - 284, Replace the
hardcoded eyebrow text in the Player component with the i18n translation key:
add "eyebrow": "KAPTER SUBTITLE PLAYER" to each locale file under
i18n/locales/*/player.json and update the Text in
apps/mobile-app/src/app/(app)/player.tsx to use the translation function (e.g.,
t('player.eyebrow') via useTranslation) instead of the literal string; ensure
useTranslation (or the project's i18n hook) is imported and used in the Player
component so the Text reads the translated value and no hardcoded strings
remain.
Comment on lines
+42
to
+47
| const sourceLabel = | ||
| source.sourceKind === "local" | ||
| ? "DEVICE" | ||
| : source.sourceKind === "cloud" | ||
| ? "STREAM" | ||
| : "SUBTITLES"; |
There was a problem hiding this comment.
Hardcoded strings violate coding guidelines.
Multiple user-facing strings are hardcoded and should use the i18n translation system:
- Source labels: "DEVICE", "STREAM", "SUBTITLES"
- Captions: "Original video unavailable...", "Subtitle-only mode", "Audio playback", "Video playback"
🔧 Proposed fix
+import { useTranslation } from "react-i18next";
export function MediaPane({
title,
thumbnailUrl,
originType,
source,
videoPlayer,
}: MediaPaneProps) {
const { theme } = useUnistyles();
+ const { t } = useTranslation("player");
// ... video branch ...
const sourceLabel =
source.sourceKind === "local"
- ? "DEVICE"
+ ? t("sourceDevice")
: source.sourceKind === "cloud"
- ? "STREAM"
- : "SUBTITLES";
+ ? t("sourceStream")
+ : t("sourceSubtitles");
// ... later in JSX ...
<Text
style={[styles.caption, { color: theme.colors.textSecondary }]}
numberOfLines={2}
>
{source.reason === "missing-local-video"
- ? "Original video unavailable on this device"
+ ? t("videoUnavailable")
: source.kind === "none"
- ? "Subtitle-only mode"
+ ? t("subtitleOnlyMode")
: source.kind === "audio"
- ? "Audio playback"
- : "Video playback"}
+ ? t("audioPlayback")
+ : t("videoPlayback")}
</Text>Add corresponding keys to apps/mobile-app/src/i18n/locales/*/player.json.
As per coding guidelines: "No hardcoded colors or strings in mobile app code".
Also applies to: 118-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/mobile-app/src/components/player/MediaPane.tsx` around lines 42 - 47,
Replace hardcoded user-facing strings in the MediaPane component with i18n keys:
update the sourceLabel computation (variable sourceLabel) to use the translation
function (e.g., t('player.device'), t('player.stream'), t('player.subtitles'))
instead of "DEVICE"/"STREAM"/"SUBTITLES", and replace the other hardcoded
captions ("Original video unavailable...", "Subtitle-only mode", "Audio
playback", "Video playback") used in MediaPane (and nearby code around the block
flagged at 118-124) with appropriate t('player.*') keys; then add those keys to
apps/mobile-app/src/i18n/locales/*/player.json for each locale so translations
are available.
Comment on lines
+57
to
+78
| if (mediaItem.originType === "YOUTUBE" && streamQuery.data) { | ||
| const { videoUrl, audioUrl } = streamQuery.data; | ||
| if (isMergedPlayableVideo(videoUrl, audioUrl)) { | ||
| return { | ||
| kind: "video", | ||
| uri: videoUrl, | ||
| sourceKind: "cloud", | ||
| canPlayVideo: true, | ||
| videoUri: videoUrl, | ||
| audioUri: audioUrl, | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| kind: "audio", | ||
| uri: audioUrl, | ||
| sourceKind: "cloud", | ||
| canPlayVideo: false, | ||
| audioUri: audioUrl, | ||
| videoUri: videoUrl, | ||
| }; | ||
| } |
There was a problem hiding this comment.
This branch disables video for split YouTube streams.
Anything except the videoUrl === audioUrl case is forced to kind: "audio", even though you already have a videoUrl. Because useMediaPlayback() only consumes source.kind/source.uri, every split-track result from the backend falls back to audio-only here. Either make the backend guarantee a single playable video URL, or carry a renderable video source through instead of downgrading to audio.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/mobile-app/src/hooks/usePlaybackSource.ts` around lines 57 - 78, The
branch handling YOUTUBE split streams incorrectly downgrades to audio whenever
isMergedPlayableVideo(videoUrl, audioUrl) is false, losing playable video even
when videoUrl exists; update the logic inside usePlaybackSource (the YOUTUBE
branch using streamQuery.data and isMergedPlayableVideo) to return a video-kind
source whenever a renderable videoUrl is available (set kind: "video", uri:
videoUrl, sourceKind: "cloud", canPlayVideo: true, videoUri: videoUrl and keep
audioUri if present), and only fall back to kind: "audio" when no videoUrl is
present or is empty/invalid. Ensure isMergedPlayableVideo is still used to
detect merged streams but do not force audio for split-track results that
include a valid videoUrl.
Comment on lines
+10
to
+15
| const { url } = await mediaService.getDownloadUrl(mediaId); | ||
| const response = await fetch(url); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`Failed to fetch subtitles: ${response.status}`); | ||
| } |
There was a problem hiding this comment.
Move subtitle artifact fetching into service layer and remove inline error text.
Line 11 performs a direct network call in the hook, and Line 14 introduces a hardcoded string. Please route this through a centralized service method and use an app-level localized/error-code approach instead of inline text.
As per coding guidelines "apps/mobile-app/src/**/*.{service,ts,tsx}: Route all API calls through centralized Axios services in the mobile app" and "apps/mobile-app/src/**/*.{ts,tsx}: No hardcoded colors or strings in mobile app code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/mobile-app/src/hooks/usePlayerSubtitles.ts` around lines 10 - 15, The
hook usePlayerSubtitles is performing a direct fetch of the subtitle artifact
and throwing a hardcoded Error string; move the network retrieval into the
centralized media service (e.g., add a method like
mediaService.fetchSubtitleArtifact or mediaService.downloadSubtitle that accepts
the mediaId/url and returns the response/body) and update the hook to call that
service method instead of fetch(url). Replace the inline throw new Error(`Failed
to fetch subtitles: ${response.status}`) with an app-level error code or
localized message (e.g., use existing localization util or ErrorCodes enum)
returned/raised by the new mediaService method so the hook only handles the
service error rather than hardcoded text.
Comment on lines
+52
to
+56
| async getVault(): Promise<Record<string, LocalVaultEntry>> { | ||
| await ensureVaultDirectory(); | ||
| const data = await AsyncStorage.getItem(VAULT_KEY); | ||
| return data ? JSON.parse(data) : {}; | ||
| }, |
There was a problem hiding this comment.
Guard vault deserialization failures.
JSON.parse on Line 55 can throw for corrupted storage and break every vault read path. Add a safe fallback to keep the app recoverable.
Proposed fix
async getVault(): Promise<Record<string, LocalVaultEntry>> {
await ensureVaultDirectory();
const data = await AsyncStorage.getItem(VAULT_KEY);
- return data ? JSON.parse(data) : {};
+ if (!data) return {};
+ try {
+ return JSON.parse(data) as Record<string, LocalVaultEntry>;
+ } catch {
+ await AsyncStorage.removeItem(VAULT_KEY);
+ return {};
+ }
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async getVault(): Promise<Record<string, LocalVaultEntry>> { | |
| await ensureVaultDirectory(); | |
| const data = await AsyncStorage.getItem(VAULT_KEY); | |
| return data ? JSON.parse(data) : {}; | |
| }, | |
| async getVault(): Promise<Record<string, LocalVaultEntry>> { | |
| await ensureVaultDirectory(); | |
| const data = await AsyncStorage.getItem(VAULT_KEY); | |
| if (!data) return {}; | |
| try { | |
| return JSON.parse(data) as Record<string, LocalVaultEntry>; | |
| } catch { | |
| await AsyncStorage.removeItem(VAULT_KEY); | |
| return {}; | |
| } | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/mobile-app/src/services/local-media-vault.ts` around lines 52 - 56, The
getVault function can throw if JSON.parse encounters corrupted data; wrap the
deserialization in a try/catch inside getVault (after await
ensureVaultDirectory() and after reading AsyncStorage.getItem(VAULT_KEY)) to
safely handle malformed JSON: on parse success return the parsed Record<string,
LocalVaultEntry>, on failure log the error, clear or reset the stored value for
VAULT_KEY (e.g., AsyncStorage.removeItem or write an empty object) and return an
empty object so all vault read paths remain recoverable; reference getVault,
ensureVaultDirectory, VAULT_KEY, AsyncStorage, and LocalVaultEntry when making
the change.
Comment on lines
+66
to
+93
| async copyIntoVault( | ||
| input: SaveLocalVaultEntryInput, | ||
| ): Promise<LocalVaultEntry> { | ||
| await ensureVaultDirectory(); | ||
|
|
||
| const fileName = sanitizeFileName(input.originalFileName); | ||
| const targetUri = `${VAULT_DIR}/${input.mediaItemId}-${Date.now()}-${fileName}`; | ||
|
|
||
| await copyAsync({ | ||
| from: input.sourceUri, | ||
| to: targetUri, | ||
| }); | ||
|
|
||
| const entry: LocalVaultEntry = { | ||
| mediaItemId: input.mediaItemId, | ||
| localUri: targetUri, | ||
| originalFileName: input.originalFileName, | ||
| mediaKind: input.mediaKind, | ||
| sizeBytes: input.sizeBytes, | ||
| durationSec: input.durationSec, | ||
| createdAt: new Date().toISOString(), | ||
| pinned: input.pinned ?? false, | ||
| }; | ||
|
|
||
| await this.saveEntry(entry); | ||
|
|
||
| return entry; | ||
| }, |
There was a problem hiding this comment.
Prevent orphaned files when replacing an existing mediaItemId entry.
Line 72 always writes a new timestamped file, but the previous file for the same mediaItemId is never deleted. Metadata is overwritten, so old files accumulate indefinitely.
Proposed fix
async copyIntoVault(
input: SaveLocalVaultEntryInput,
): Promise<LocalVaultEntry> {
await ensureVaultDirectory();
+ const existing = await this.getEntry(input.mediaItemId);
+ if (existing?.localUri) {
+ await deleteAsync(existing.localUri, { idempotent: true });
+ }
+
const fileName = sanitizeFileName(input.originalFileName);
const targetUri = `${VAULT_DIR}/${input.mediaItemId}-${Date.now()}-${fileName}`;
await copyAsync({
from: input.sourceUri,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async copyIntoVault( | |
| input: SaveLocalVaultEntryInput, | |
| ): Promise<LocalVaultEntry> { | |
| await ensureVaultDirectory(); | |
| const fileName = sanitizeFileName(input.originalFileName); | |
| const targetUri = `${VAULT_DIR}/${input.mediaItemId}-${Date.now()}-${fileName}`; | |
| await copyAsync({ | |
| from: input.sourceUri, | |
| to: targetUri, | |
| }); | |
| const entry: LocalVaultEntry = { | |
| mediaItemId: input.mediaItemId, | |
| localUri: targetUri, | |
| originalFileName: input.originalFileName, | |
| mediaKind: input.mediaKind, | |
| sizeBytes: input.sizeBytes, | |
| durationSec: input.durationSec, | |
| createdAt: new Date().toISOString(), | |
| pinned: input.pinned ?? false, | |
| }; | |
| await this.saveEntry(entry); | |
| return entry; | |
| }, | |
| async copyIntoVault( | |
| input: SaveLocalVaultEntryInput, | |
| ): Promise<LocalVaultEntry> { | |
| await ensureVaultDirectory(); | |
| const existing = await this.getEntry(input.mediaItemId); | |
| if (existing?.localUri) { | |
| await deleteAsync(existing.localUri, { idempotent: true }); | |
| } | |
| const fileName = sanitizeFileName(input.originalFileName); | |
| const targetUri = `${VAULT_DIR}/${input.mediaItemId}-${Date.now()}-${fileName}`; | |
| await copyAsync({ | |
| from: input.sourceUri, | |
| to: targetUri, | |
| }); | |
| const entry: LocalVaultEntry = { | |
| mediaItemId: input.mediaItemId, | |
| localUri: targetUri, | |
| originalFileName: input.originalFileName, | |
| mediaKind: input.mediaKind, | |
| sizeBytes: input.sizeBytes, | |
| durationSec: input.durationSec, | |
| createdAt: new Date().toISOString(), | |
| pinned: input.pinned ?? false, | |
| }; | |
| await this.saveEntry(entry); | |
| return entry; | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/mobile-app/src/services/local-media-vault.ts` around lines 66 - 93, The
current copyIntoVault always writes a new timestamped file and overwrites
metadata, leaving the old file orphaned; modify copyIntoVault to (1) after
creating targetUri with copyAsync, call this.saveEntry(entry), (2) if there is
an existing entry for the same mediaItemId (lookup via something like
this.getEntry(input.mediaItemId) or this.findEntryByMediaItemId before copying),
store its localUri, and after successful save unlink the previous file (use
fs/promises.unlink or your project's removeAsync) to avoid orphaned files, and
(3) on failure to saveEntry, delete the newly copied targetUri so you don't
create a new orphan—use the existing symbols copyAsync, saveEntry, VAULT_DIR,
sanitizeFileName, SaveLocalVaultEntryInput, and LocalVaultEntry to locate and
implement this behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/mobile-app/src/services/local-media-vault.ts (1)
72-87: Error handling around file copy is optional but recommended for clarity.The
File.copy()method inexpo-file-systemis synchronous and throws synchronously on failure. The current code is safe—ifcopy()fails, the exception propagates beforesaveEntry()is called, preventing orphaned metadata.However, wrapping the copy in
try/catchwould improve code clarity and enable graceful error handling or informative logging, making the intent explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile-app/src/services/local-media-vault.ts` around lines 72 - 87, Wrap the synchronous File.copy call in a try/catch so copy failures are explicitly handled before saveEntry is called; locate the block creating sourceFile with new File(input.sourceUri) and calling sourceFile.copy(targetFile), catch any thrown error, log an informative message (use existing logger if available, otherwise console.error) including input.sourceUri and targetFile.uri, and then rethrow the error to prevent calling this.saveEntry(entry) on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/mobile-app/src/services/local-media-vault.ts`:
- Around line 40-46: Export a new clearVault() method on the localMediaVault
object that: calls ensureVaultDirectory(), removes the stored VAULT_KEY entry
from AsyncStorage, and deletes all files in the vault directory (so no leftover
media files remain); update the export to include clearVault alongside existing
methods (getVault) and then call localMediaVault.clearVault() from the logout
flow in auth.store.ts alongside clearTokens() to ensure user media is fully
purged. Ensure the implementation references the existing VAULT_KEY constant,
ensureVaultDirectory(), and the localMediaVault export so the logout integration
can import and invoke clearVault.
---
Nitpick comments:
In `@apps/mobile-app/src/services/local-media-vault.ts`:
- Around line 72-87: Wrap the synchronous File.copy call in a try/catch so copy
failures are explicitly handled before saveEntry is called; locate the block
creating sourceFile with new File(input.sourceUri) and calling
sourceFile.copy(targetFile), catch any thrown error, log an informative message
(use existing logger if available, otherwise console.error) including
input.sourceUri and targetFile.uri, and then rethrow the error to prevent
calling this.saveEntry(entry) on failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad2312b3-9f09-4559-917b-1d2426feb4b7
📒 Files selected for processing (3)
apps/mobile-app/src/services/local-media-vault.tsapps/mobile-app/src/theme/light.tsapps/mobile-app/src/theme/tokens.ts
✅ Files skipped from review due to trivial changes (2)
- apps/mobile-app/src/theme/tokens.ts
- apps/mobile-app/src/theme/light.ts
Comment on lines
+40
to
+46
| export const localMediaVault = { | ||
| /** Get the whole vault map */ | ||
| async getVault(): Promise<Record<string, LocalVaultEntry>> { | ||
| await ensureVaultDirectory(); | ||
| const data = await AsyncStorage.getItem(VAULT_KEY); | ||
| return data ? JSON.parse(data) : {}; | ||
| }, |
There was a problem hiding this comment.
Consider exporting a clearVault() method for logout/user-switch scenarios.
Based on the relevant code snippets, clearTokens() called during logout only clears SecureStore, leaving the @kapter/local-vault AsyncStorage key and associated files intact. This creates a data isolation issue where the next user session could access the previous user's local media entries.
Suggested addition
+ /** Clear entire vault (for logout/user-switch) */
+ async clearVault(): Promise<void> {
+ const vault = await this.getVault();
+ for (const entry of Object.values(vault)) {
+ const localFile = new File(entry.localUri);
+ if (localFile.exists) {
+ localFile.delete();
+ }
+ }
+ await AsyncStorage.removeItem(VAULT_KEY);
+ },Then integrate with the logout flow in auth.store.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/mobile-app/src/services/local-media-vault.ts` around lines 40 - 46,
Export a new clearVault() method on the localMediaVault object that: calls
ensureVaultDirectory(), removes the stored VAULT_KEY entry from AsyncStorage,
and deletes all files in the vault directory (so no leftover media files
remain); update the export to include clearVault alongside existing methods
(getVault) and then call localMediaVault.clearVault() from the logout flow in
auth.store.ts alongside clearTokens() to ensure user media is fully purged.
Ensure the implementation references the existing VAULT_KEY constant,
ensureVaultDirectory(), and the localMediaVault export so the logout integration
can import and invoke clearVault.
…nstead of waiting final translated json
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
apps/mobile-app/src/components/player/PlayerControls.tsx (4)
198-208:⚠️ Potential issue | 🟠 MajorComplete the adjustable seek bar accessibility contract.
With
accessibilityRole="adjustable"in place, this control still needsaccessibilityValueplus increment/decrement actions and a handler. Without them, screen-reader users can focus the seek bar but still can't scrub meaningfully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile-app/src/components/player/PlayerControls.tsx` around lines 198 - 208, The AnimatedView acting as the seek bar has accessibilityRole="adjustable" but lacks accessibilityValue, accessibilityActions and an onAccessibilityAction handler; update the AnimatedView to provide accessibilityValue (e.g., current value, min, max using your currentPosition and duration state/props), add accessibilityActions = [{name: 'increment'}, {name: 'decrement'}], and implement onAccessibilityAction that translates increment/decrement into a small seek change (call your existing seek/seekTo handler or a new helper like seekByDelta(delta)) while honoring disabled; reference the AnimatedView component, the disabled prop/state, and your current position/duration variables when wiring these values.
34-38:⚠️ Potential issue | 🟡 MinorHandle hour-long timestamps in both player time formatters.
formatTime(3665)still renders61:05here, and the copied helper inapps/mobile-app/src/app/(app)/player.tsxhas the same missing hours branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile-app/src/components/player/PlayerControls.tsx` around lines 34 - 38, formatTime currently only formats minutes and seconds so inputs >= 3600s produce "61:05" instead of "1:01:05"; update the formatTime function (and the duplicate helper in apps/mobile-app/src/app/(app)/player.tsx) to detect hours (hours = Math.floor(seconds / 3600)), subtract hours from remaining seconds, then format as "H:MM:SS" when hours > 0 (pad minutes and seconds to two digits) and as "M:SS" when hours === 0, using the existing padStart logic for minutes/seconds and preserving Math.floor usage.
264-288:⚠️ Potential issue | 🟠 MajorLabel the icon-only play/pause control.
This
Pressablestill has no localized accessibility label/role, so assistive tech only gets an unlabeled transport button while the adjacent skip controls are announced correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile-app/src/components/player/PlayerControls.tsx` around lines 264 - 288, The play/pause Pressable lacks an accessibility label/role; update the Pressable (the component using onTogglePlayback, isCoveragePending, isPlaying, and styles.playButton) to provide a localized accessibilityLabel (e.g., "Play" or "Pause" via i18n) and set accessibilityRole="button" (and optionally accessibilityState={{ disabled: true }} and an accessibilityHint) so screen readers announce the control correctly when isCoveragePending or isPlaying changes.
242-252:⚠️ Potential issue | 🟡 MinorLocalize the full playback-speed label.
The inline
"x"at Line 251 is still part of user-facing copy, so translators cannot control the suffix, spacing, or locale-specific formatting of the full label.As per coding guidelines "
apps/mobile-app/src/**/*.{ts,tsx}: No hardcoded colors or strings in mobile app code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile-app/src/components/player/PlayerControls.tsx` around lines 242 - 252, The displayed playback speed currently appends a hardcoded "x" (in PlayerControls.tsx near playbackSpeed and styles.sideValue), which must be localized; replace the inline suffix with a translatable label (e.g., use the app i18n function like t('player.playbackSpeed', { value: formattedSpeed }) or a similar locale-aware formatter) so translators control spacing/suffix and formatting, format playbackSpeed using the app's locale-aware number formatter before passing it into the translation placeholder, and add/update the corresponding translation key in the locale files; remove the hardcoded "x" from the component so no user-facing strings remain inline.apps/mobile-app/src/app/(app)/player.tsx (1)
352-371:⚠️ Potential issue | 🟡 MinorLocalize the remaining header copy.
accessibilityLabel="Back"andKAPTER SUBTITLE PLAYERare still inline strings, so this header is not fully translatable yet.As per coding guidelines "
apps/mobile-app/src/**/*.{ts,tsx}: No hardcoded colors or strings in mobile app code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile-app/src/app/`(app)/player.tsx around lines 352 - 371, The header still contains hardcoded strings: the IconButton accessibilityLabel and the eyebrow Text ("KAPTER SUBTITLE PLAYER"); update both to use your app's localization utility (e.g., import/useTranslation or i18n.t) instead of inline text — replace accessibilityLabel on the IconButton and the Text children for the header eyebrow (styles.headerEyebrow) with localized keys like "player.back" and "player.subtitle" and add those keys to the locale JSONs so the header is fully translatable.apps/mobile-app/src/hooks/usePlayerSubtitles.ts (1)
17-37:⚠️ Potential issue | 🟠 MajorMove subtitle downloads out of the hook.
fetchSubtitleOutput()andfetchArtifactJson()are still doing raw network I/O here and still throw inline error text. That keeps transport/error policy duplicated in the hook instead of the centralized service layer.As per coding guidelines "
apps/mobile-app/src/**/*.{service,ts,tsx}: Route all API calls through centralized Axios services in the mobile app" and "apps/mobile-app/src/**/*.{ts,tsx}: No hardcoded colors or strings in mobile app code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile-app/src/hooks/usePlayerSubtitles.ts` around lines 17 - 37, fetchSubtitleOutput and fetchArtifactJson perform raw fetch calls and throw inline Error strings; move their network I/O into the centralized mobile Axios service and replace inline error text with service-level error handling. Specifically, remove the fetch calls from the hook and instead call new or existing methods on mediaService (e.g., mediaService.getSubtitleOutput(mediaId) and mediaService.getArtifactJson<T>(url)) that use the app's Axios instance, normalize responses there (call normalizeSubtitleOutput after the service returns data), and have the service throw or return structured errors (not raw hardcoded strings) so the hook only consumes results or well-typed error objects. Ensure the hook imports and uses those service methods and does not perform any direct network I/O or hardcoded error messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/mobile-app/src/app/`(app)/player.tsx:
- Around line 374-383: The IconButton currently uses mediaItem?.originUrl to
switch between handleOpenYoutube and opening the layers modal
(setLayersVisible), which hides the modal when originUrl exists and also lacks
an accessibility label; update the IconButton to always expose the layers modal
via a stable interaction (for example keep onPress for handleOpenYoutube but add
onLongPress or a separate control that calls setLayersVisible, or invert logic
so a dedicated control opens the modal) and add an accessibilityLabel prop
(e.g., "More options" or "Toggle layers") to the IconButton so screen readers
can identify it; change touches in the IconButton usage in player.tsx (the
IconButton element, mediaItem?.originUrl, handleOpenYoutube, setLayersVisible)
accordingly.
In `@apps/mobile-app/src/hooks/usePlayerSubtitles.ts`:
- Around line 69-96: The query key for batchQuery is unstable because it embeds
translatedBatches; change it to a stable key (e.g., ["player-subtitles-batches",
mediaId]) and update the queryFn so it accumulates new batches instead of
re-fetching everything: inside the batchQuery queryFn (the function that
currently loops over translatedBatches and calls fetchArtifactJson), read
existing segments from the cache via
queryClient.getQueryData(["player-subtitles-batches", mediaId]) or accept the
previousData param, compute which translatedBatches URLs are not yet present,
fetch only those URLs with fetchArtifactJson, merge normalized segments into the
existing Map used for segmentsByIndex, then return the merged sorted segment
array; keep the enabled and staleTime logic but ensure translatedBatches is used
only to discover new URLs to fetch, not as part of the queryKey.
---
Duplicate comments:
In `@apps/mobile-app/src/app/`(app)/player.tsx:
- Around line 352-371: The header still contains hardcoded strings: the
IconButton accessibilityLabel and the eyebrow Text ("KAPTER SUBTITLE PLAYER");
update both to use your app's localization utility (e.g., import/useTranslation
or i18n.t) instead of inline text — replace accessibilityLabel on the IconButton
and the Text children for the header eyebrow (styles.headerEyebrow) with
localized keys like "player.back" and "player.subtitle" and add those keys to
the locale JSONs so the header is fully translatable.
In `@apps/mobile-app/src/components/player/PlayerControls.tsx`:
- Around line 198-208: The AnimatedView acting as the seek bar has
accessibilityRole="adjustable" but lacks accessibilityValue,
accessibilityActions and an onAccessibilityAction handler; update the
AnimatedView to provide accessibilityValue (e.g., current value, min, max using
your currentPosition and duration state/props), add accessibilityActions =
[{name: 'increment'}, {name: 'decrement'}], and implement onAccessibilityAction
that translates increment/decrement into a small seek change (call your existing
seek/seekTo handler or a new helper like seekByDelta(delta)) while honoring
disabled; reference the AnimatedView component, the disabled prop/state, and
your current position/duration variables when wiring these values.
- Around line 34-38: formatTime currently only formats minutes and seconds so
inputs >= 3600s produce "61:05" instead of "1:01:05"; update the formatTime
function (and the duplicate helper in apps/mobile-app/src/app/(app)/player.tsx)
to detect hours (hours = Math.floor(seconds / 3600)), subtract hours from
remaining seconds, then format as "H:MM:SS" when hours > 0 (pad minutes and
seconds to two digits) and as "M:SS" when hours === 0, using the existing
padStart logic for minutes/seconds and preserving Math.floor usage.
- Around line 264-288: The play/pause Pressable lacks an accessibility
label/role; update the Pressable (the component using onTogglePlayback,
isCoveragePending, isPlaying, and styles.playButton) to provide a localized
accessibilityLabel (e.g., "Play" or "Pause" via i18n) and set
accessibilityRole="button" (and optionally accessibilityState={{ disabled: true
}} and an accessibilityHint) so screen readers announce the control correctly
when isCoveragePending or isPlaying changes.
- Around line 242-252: The displayed playback speed currently appends a
hardcoded "x" (in PlayerControls.tsx near playbackSpeed and styles.sideValue),
which must be localized; replace the inline suffix with a translatable label
(e.g., use the app i18n function like t('player.playbackSpeed', { value:
formattedSpeed }) or a similar locale-aware formatter) so translators control
spacing/suffix and formatting, format playbackSpeed using the app's locale-aware
number formatter before passing it into the translation placeholder, and
add/update the corresponding translation key in the locale files; remove the
hardcoded "x" from the component so no user-facing strings remain inline.
In `@apps/mobile-app/src/hooks/usePlayerSubtitles.ts`:
- Around line 17-37: fetchSubtitleOutput and fetchArtifactJson perform raw fetch
calls and throw inline Error strings; move their network I/O into the
centralized mobile Axios service and replace inline error text with
service-level error handling. Specifically, remove the fetch calls from the hook
and instead call new or existing methods on mediaService (e.g.,
mediaService.getSubtitleOutput(mediaId) and
mediaService.getArtifactJson<T>(url)) that use the app's Axios instance,
normalize responses there (call normalizeSubtitleOutput after the service
returns data), and have the service throw or return structured errors (not raw
hardcoded strings) so the hook only consumes results or well-typed error
objects. Ensure the hook imports and uses those service methods and does not
perform any direct network I/O or hardcoded error messages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3548fbb1-92f7-467d-99ac-27d4b67a2142
📒 Files selected for processing (8)
apps/mobile-app/src/app/(app)/player.tsxapps/mobile-app/src/components/index.tsapps/mobile-app/src/components/player/PlayerControls.tsxapps/mobile-app/src/components/player/StreamingTailIndicator.tsxapps/mobile-app/src/components/player/SubtitleRow.tsxapps/mobile-app/src/hooks/usePlayerSubtitles.tsapps/mobile-app/src/i18n/locales/en/player.jsonapps/mobile-app/src/i18n/locales/vi/player.json
✅ Files skipped from review due to trivial changes (2)
- apps/mobile-app/src/i18n/locales/vi/player.json
- apps/mobile-app/src/i18n/locales/en/player.json
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/mobile-app/src/components/index.ts
- apps/mobile-app/src/components/player/SubtitleRow.tsx
Comment on lines
+374
to
+383
| <IconButton | ||
| name={mediaItem?.originUrl ? "open-outline" : "ellipsis-horizontal"} | ||
| size={24} | ||
| color={headerTextColor} | ||
| onPress={ | ||
| mediaItem?.originUrl | ||
| ? handleOpenYoutube | ||
| : () => setLayersVisible(true) | ||
| } | ||
| > | ||
| <Ionicons name="arrow-back" size={22} color={theme.colors.text} /> | ||
| </Pressable> | ||
| <Text style={[styles.title, { color: theme.colors.text }]}> | ||
| {t("playerPlaceholder.title")} | ||
| </Text> | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "player.tsx" -type f | grep mobile-app | head -5Repository: KMA-Final-Project/project
Length of output: 51
🏁 Script executed:
wc -l apps/mobile-app/src/app/\(app\)/player.tsxRepository: KMA-Final-Project/project
Length of output: 113
🏁 Script executed:
cat -n apps/mobile-app/src/app/\(app\)/player.tsx | sed -n '370,390p'Repository: KMA-Final-Project/project
Length of output: 786
🏁 Script executed:
cat -n apps/mobile-app/src/app/\(app\)/player.tsx | sed -n '520,540p'Repository: KMA-Final-Project/project
Length of output: 774
🏁 Script executed:
cat -n apps/mobile-app/src/app/\(app\)/player.tsx | sed -n '1,50p'Repository: KMA-Final-Project/project
Length of output: 2037
🏁 Script executed:
cat -n apps/mobile-app/src/app/\(app\)/player.tsx | sed -n '100,250p'Repository: KMA-Final-Project/project
Length of output: 5103
🏁 Script executed:
grep -n "handleOpenYoutube" apps/mobile-app/src/app/\(app\)/player.tsxRepository: KMA-Final-Project/project
Length of output: 202
🏁 Script executed:
rg "IconButton" apps/mobile-app/src/components --type tsx --type ts -A 5 | head -50Repository: KMA-Final-Project/project
Length of output: 96
🏁 Script executed:
rg "accessibilityLabel" apps/mobile-app/src --type tsx --type ts | head -20Repository: KMA-Final-Project/project
Length of output: 96
🏁 Script executed:
rg "accessibilityLabel" apps/mobile-app/src -t ts -t tsx | head -20Repository: KMA-Final-Project/project
Length of output: 96
🏁 Script executed:
fd "IconButton" apps/mobile-app/src/components -type fRepository: KMA-Final-Project/project
Length of output: 239
🏁 Script executed:
find apps/mobile-app/src/components -name "*IconButton*" -o -name "*icon-button*"Repository: KMA-Final-Project/project
Length of output: 114
🏁 Script executed:
rg "interface.*IconButton\|type.*IconButton\|function.*IconButton" apps/mobile-app/src/components -A 10Repository: KMA-Final-Project/project
Length of output: 51
🏁 Script executed:
cat -n apps/mobile-app/src/components/IconButton.tsxRepository: KMA-Final-Project/project
Length of output: 2505
🏁 Script executed:
grep -n "accessibilityLabel\|accessible" apps/mobile-app/src/components/IconButton.tsxRepository: KMA-Final-Project/project
Length of output: 176
🏁 Script executed:
cat -n apps/mobile-app/src/app/\(app\)/player.tsx | sed -n '218,225p'Repository: KMA-Final-Project/project
Length of output: 286
🏁 Script executed:
rg "WebBrowser" apps/mobile-app/src --context 2 | head -30Repository: KMA-Final-Project/project
Length of output: 782
🏁 Script executed:
cat -n apps/mobile-app/src/app/\(app\)/player.tsx | sed -n '1,30p'Repository: KMA-Final-Project/project
Length of output: 977
🏁 Script executed:
find apps/mobile-app -name "*.service.ts" -o -name "*service*" | grep -v node_modulesRepository: KMA-Final-Project/project
Length of output: 237
🏁 Script executed:
cat apps/mobile-app/src/services/auth.services.tsRepository: KMA-Final-Project/project
Length of output: 1039
🏁 Script executed:
cat apps/mobile-app/src/services/media.services.tsRepository: KMA-Final-Project/project
Length of output: 2700
🏁 Script executed:
rg "axios\|fetch\|WebBrowser" apps/mobile-app/src/servicesRepository: KMA-Final-Project/project
Length of output: 51
Add accessibilityLabel to the header button and reconsider the dual-functionality design.
The IconButton conditionally switches between opening the browser (handleOpenYoutube) and showing the layer toggle modal based on mediaItem.originUrl. When a source URL exists, the modal becomes inaccessible—users lose the ability to toggle phonetic/translation/karaoke layers. Additionally, the button lacks an accessibility label, which is required for icon-only buttons. The IconButton component supports the accessibilityLabel prop but it is not being passed here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/mobile-app/src/app/`(app)/player.tsx around lines 374 - 383, The
IconButton currently uses mediaItem?.originUrl to switch between
handleOpenYoutube and opening the layers modal (setLayersVisible), which hides
the modal when originUrl exists and also lacks an accessibility label; update
the IconButton to always expose the layers modal via a stable interaction (for
example keep onPress for handleOpenYoutube but add onLongPress or a separate
control that calls setLayersVisible, or invert logic so a dedicated control
opens the modal) and add an accessibilityLabel prop (e.g., "More options" or
"Toggle layers") to the IconButton so screen readers can identify it; change
touches in the IconButton usage in player.tsx (the IconButton element,
mediaItem?.originUrl, handleOpenYoutube, setLayersVisible) accordingly.
Comment on lines
+69
to
+96
| const batchQuery = useQuery({ | ||
| queryKey: [ | ||
| "player-subtitles-batches", | ||
| mediaId, | ||
| translatedBatches.map((batch) => `${batch.batchIndex}:${batch.url}`), | ||
| ], | ||
| queryFn: async () => { | ||
| const segmentsByIndex = new Map<number, Sentence>(); | ||
|
|
||
| for (const batch of translatedBatches) { | ||
| const payload = await fetchArtifactJson<TranslatedBatch>(batch.url); | ||
|
|
||
| for (const segment of payload.segments.map(normalizeSentence)) { | ||
| if (segment.segment_index == null) { | ||
| continue; | ||
| } | ||
|
|
||
| segmentsByIndex.set(segment.segment_index, segment); | ||
| } | ||
| } | ||
|
|
||
| return [...segmentsByIndex.entries()] | ||
| .sort((left, right) => left[0] - right[0]) | ||
| .map(([, segment]) => segment); | ||
| }, | ||
| enabled: Boolean(mediaId && !finalUrl && translatedBatches.length > 0), | ||
| staleTime: Infinity, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and get its structure
fd -t f "usePlayerSubtitles.ts" apps/mobile-app/Repository: KMA-Final-Project/project
Length of output: 116
🏁 Script executed:
# Read the file to verify the code snippet
wc -l apps/mobile-app/src/hooks/usePlayerSubtitles.tsRepository: KMA-Final-Project/project
Length of output: 120
🏁 Script executed:
# Read lines around 69-96 to verify the snippet
sed -n '60,100p' apps/mobile-app/src/hooks/usePlayerSubtitles.tsRepository: KMA-Final-Project/project
Length of output: 1270
🏁 Script executed:
# Check the entire file to understand translatedBatches origin and flow
cat -n apps/mobile-app/src/hooks/usePlayerSubtitles.tsRepository: KMA-Final-Project/project
Length of output: 5400
Use a stable query key and accumulate batches instead of resetting on each artifact discovery.
The queryKey at line 73 is derived from the entire translatedBatches array mapped as a single unit. When new batches arrive in the artifact inventory, this queryKey changes completely, causing React Query to treat it as a new query and invalidate the cache. The queryFn then re-fetches all previously loaded batches in the serial loop (lines 78–88) before processing new ones, resulting in unnecessary network requests and potential subtitle dropout when playback resumes.
Keep the query key stable (e.g., keyed only on mediaId) and refactor the query logic to accumulate newly discovered batch URLs instead of refetching the entire set.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/mobile-app/src/hooks/usePlayerSubtitles.ts` around lines 69 - 96, The
query key for batchQuery is unstable because it embeds translatedBatches; change
it to a stable key (e.g., ["player-subtitles-batches", mediaId]) and update the
queryFn so it accumulates new batches instead of re-fetching everything: inside
the batchQuery queryFn (the function that currently loops over translatedBatches
and calls fetchArtifactJson), read existing segments from the cache via
queryClient.getQueryData(["player-subtitles-batches", mediaId]) or accept the
previousData param, compute which translatedBatches URLs are not yet present,
fetch only those URLs with fetchArtifactJson, merge normalized segments into the
existing Map used for segmentsByIndex, then return the merged sorted segment
array; keep the enabled and staleTime logic but ensure translatedBatches is used
only to discover new URLs to fetch, not as part of the queryKey.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
apps/backend-api/src/modules/media/yt-dlp.service.ts (1)
217-229: Consider distinguishing JSON parse errors for better debugging.If
stdoutis truncated (e.g., due tomaxBufferoverflow) or malformed,JSON.parsewill throw aSyntaxError. The current error handling treats this the same as yt-dlp execution failures, making root-cause diagnosis harder.♻️ Suggested improvement
private async loadJson( youtubeUrl: string, args: string[], timeout: number, action: string, ): Promise<YtDlpJson> { try { - const { stdout } = await execFileAsync('yt-dlp', args, { timeout }); - return JSON.parse(stdout) as YtDlpJson; + const { stdout } = await execFileAsync('yt-dlp', args, { + timeout, + maxBuffer: 10 * 1024 * 1024, + }); + try { + return JSON.parse(stdout) as YtDlpJson; + } catch (parseErr) { + this.logger.error(`Failed to parse yt-dlp output (${stdout.length} bytes)`); + throw new Error(`yt-dlp returned invalid JSON while trying to ${action}`); + } } catch (error) { this.throwYtDlpError(error, action, youtubeUrl); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend-api/src/modules/media/yt-dlp.service.ts` around lines 217 - 229, The loadJson method conflates yt-dlp execution errors and JSON.parse failures; wrap JSON.parse(stdout) in its own try/catch (after awaiting execFileAsync) so you can detect SyntaxError or parse failures separately, and call throwYtDlpError (or throw a more specific error) with the parse error details and the raw stdout to aid debugging; keep execFileAsync and the outer catch for execution/timeouts but ensure parse errors include stdout, the action, and youtubeUrl for clear diagnostics.apps/mobile-app/src/hooks/useMedia.ts (1)
99-101: Consider documenting whyrefetchOnMount: "always"overrides socket-first defaults.This overrides the
refetchOnMount: falsefromsocketFirstQueryOptions. While the change ensures fresh artifact data on hook remount (useful after navigation), it deviates from the socket-first pattern used elsewhere. A brief comment explaining the rationale would help future maintainers.📝 Suggested comment
return useQuery({ queryKey: mediaKeys.artifacts(id ?? ""), queryFn: () => mediaService.getArtifacts(id!), enabled: !!id, ...socketFirstQueryOptions, + // Override socket-first default: always refresh artifacts on mount to ensure + // player/detail screens have the latest batch/final state after navigation refetchOnMount: "always", });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile-app/src/hooks/useMedia.ts` around lines 99 - 101, Add a brief inline comment in useMedia.ts next to the refetchOnMount: "always" override (where socketFirstQueryOptions is spread) explaining why it intentionally overrides the socket-first default (refetchOnMount: false) — e.g., to ensure fresh artifact data on hook remounts after navigation — so future maintainers understand the rationale when looking at socketFirstQueryOptions and the useMedia hook.apps/mobile-app/src/components/media/UploadModals.tsx (1)
70-75: Use i18n for error alert strings.The Alert title
"Failed"and fallback message"An unexpected error occurred."are hardcoded. Consider using translation keys for consistency with the rest of the app.♻️ Suggested fix
Alert.alert( - "Failed", + t("common.error"), error?.response?.data?.message ?? error?.message ?? - "An unexpected error occurred.", + t("errors.unexpected"), );As per coding guidelines "
apps/mobile-app/src/**/*.{ts,tsx}: No hardcoded colors or strings in mobile app code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile-app/src/components/media/UploadModals.tsx` around lines 70 - 75, The Alert.alert call uses hardcoded strings ("Failed" and "An unexpected error occurred.") — replace them with i18n translation keys using the app's localization helper (e.g., useTranslation or i18n.t) so the title and fallback message come from the locale files; update the Alert.alert invocation in UploadModals.tsx to import and use the translation function (reference Alert.alert) and use appropriate keys like common.failed and common.unexpectedError (or the project's standard keys), keeping the error-specific message fallback as-is but wrapped with the translator.apps/mobile-app/src/components/media/YouTubeModal.tsx (1)
103-103: Use i18n for user-facing strings.Several hardcoded strings should use translation keys:
- Line 103:
"Invalid YouTube URL"- Line 249:
"Paste"- Line 299:
"Submit"♻️ Suggested changes
- setError("Invalid YouTube URL"); + setError(t("upload.youtube.invalidUrl"));- Paste + {t("common.paste")}- <Text style={styles.btnPrimaryText}>Submit</Text> + <Text style={styles.btnPrimaryText}>{t("common.submit")}</Text>As per coding guidelines "
apps/mobile-app/src/**/*.{ts,tsx}: No hardcoded colors or strings in mobile app code".Also applies to: 249-249, 299-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile-app/src/components/media/YouTubeModal.tsx` at line 103, Replace hardcoded user-facing strings in YouTubeModal with i18n keys: import/use the translation hook in the YouTubeModal component (e.g., const { t } = useTranslation()) and change setError("Invalid YouTube URL") to setError(t('youtube.invalidUrl')), the "Paste" label to t('youtube.paste'), and the "Submit" label to t('youtube.submit'); add the corresponding keys and translations to the app locale files and update any tests or snapshots that assert these exact strings.apps/ai-engine/tests/test_minio_public_presign.py (1)
3-9: DeclareinstancesasClassVar.Line 9 is intentional shared recorder state, but annotating it as
list[...]makes it look like an instance field and trips Ruff's mutable-class-attribute warning. Marking it asClassVar[...]keeps the intent explicit.🧹 Proposed fix
from datetime import timedelta +from typing import ClassVar import src.minio_client as minio_mod class RecordingMinio: - instances: list["RecordingMinio"] = [] + instances: ClassVar[list["RecordingMinio"]] = []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ai-engine/tests/test_minio_public_presign.py` around lines 3 - 9, The shared recorder list on RecordingMinio is a class-level mutable attribute but is annotated as an instance field; import typing.ClassVar and change the type of instances to ClassVar[list["RecordingMinio"]] so Ruff no longer flags it as a mutable instance attribute and the intent remains explicit (update the RecordingMinio class and add the ClassVar import).apps/mobile-app/src/hooks/useSocketSync.ts (1)
166-173: Coalesce artifact refetches per media item.Every
media_chunk_readyandmedia_batch_readyevent now invalidates/media/:id/artifacts. On long jobs, that can turn the socket stream into a burst of REST fetches while the summary is already patched locally. A small debounce or in-flight guard aroundrefreshArtifacts()would keep this flow socket-first without hammering the artifacts endpoint.Based on learnings: Applies to apps/mobile-app/src//{processing,detail,status,socket}//*.{ts,tsx} : Processing/detail flows are socket-first; avoid reintroducing aggressive polling on status/artifact queries.
Also applies to: 240-290
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/mobile-app/src/hooks/useSocketSync.ts` around lines 166 - 173, The refreshArtifacts callback in useSocketSync.ts is causing excessive artifact refetches on rapid socket events; wrap or replace it with a per-mediaId coalescing guard (e.g., a Map tracking debounce timers or in-flight flags keyed by mediaId) so repeated calls within a short window debounce into a single queryClient.invalidateQueries(mediaKeys.artifacts(mediaId)) call; update the refreshArtifacts implementation and any callers (e.g., handlers for media_chunk_ready/media_batch_ready) to use this coalescing function and choose a short debounce (e.g., 300–1000ms) or clear the flag when the invalidate completes to avoid hammering the artifacts endpoint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/mobile-app/src/hooks/usePlayerSubtitles.ts`:
- Around line 30-38: fetchArtifactJson is doing a raw fetch with a hardcoded
error string; move this logic into the centralized media service and replace
direct calls. Implement mediaService.fetchArtifactJson<T>(url: string) that uses
the app's Axios service wrapper, throws a typed ApiError (e.g.,
'ARTIFACT_FETCH_FAILED') with the response status on non-OK responses, and
returns parsed JSON; then update usePlayerSubtitles to call
mediaService.fetchArtifactJson instead of the local fetchArtifactJson helper.
Ensure no hardcoded strings remain and that error handling uses the app-level
ApiError and logging conventions.
In `@apps/mobile-app/src/hooks/useSocketSync.ts`:
- Around line 215-235: The patch currently always sets sourceLanguage to
event.sourceLanguage ?? currentStatus?.sourceLanguage, which can insert
undefined and overwrite existing values; update the logic in useSocketSync.ts
where the progress patch is built (see variables: currentStatus, event, patch)
to only include the sourceLanguage property when event.sourceLanguage is
explicitly defined (truthy or non-null) so mergeLivePatch() won't erase an
existing sourceLanguage in the cache; implement this by conditionally adding
sourceLanguage to the Partial<MediaItem> patch (rather than always setting it to
a possibly undefined value).
In `@checkpoint.md`:
- Around line 89-92: The checkpoint.md tests tree is missing the new test file
test_minio_public_presign.py introduced in the PR; update the tests listing by
adding "test_minio_public_presign.py # MinIO public presign behavior" into the
same tests subtree where test_event_discipline.py,
test_first_batch_streaming.py, test_prewarm_startup.py, and
test_streaming_contracts.py are listed, preserving the existing
formatting/comment style and ordering convention so the checkpoint inventory
reflects the new test.
---
Nitpick comments:
In `@apps/ai-engine/tests/test_minio_public_presign.py`:
- Around line 3-9: The shared recorder list on RecordingMinio is a class-level
mutable attribute but is annotated as an instance field; import typing.ClassVar
and change the type of instances to ClassVar[list["RecordingMinio"]] so Ruff no
longer flags it as a mutable instance attribute and the intent remains explicit
(update the RecordingMinio class and add the ClassVar import).
In `@apps/backend-api/src/modules/media/yt-dlp.service.ts`:
- Around line 217-229: The loadJson method conflates yt-dlp execution errors and
JSON.parse failures; wrap JSON.parse(stdout) in its own try/catch (after
awaiting execFileAsync) so you can detect SyntaxError or parse failures
separately, and call throwYtDlpError (or throw a more specific error) with the
parse error details and the raw stdout to aid debugging; keep execFileAsync and
the outer catch for execution/timeouts but ensure parse errors include stdout,
the action, and youtubeUrl for clear diagnostics.
In `@apps/mobile-app/src/components/media/UploadModals.tsx`:
- Around line 70-75: The Alert.alert call uses hardcoded strings ("Failed" and
"An unexpected error occurred.") — replace them with i18n translation keys using
the app's localization helper (e.g., useTranslation or i18n.t) so the title and
fallback message come from the locale files; update the Alert.alert invocation
in UploadModals.tsx to import and use the translation function (reference
Alert.alert) and use appropriate keys like common.failed and
common.unexpectedError (or the project's standard keys), keeping the
error-specific message fallback as-is but wrapped with the translator.
In `@apps/mobile-app/src/components/media/YouTubeModal.tsx`:
- Line 103: Replace hardcoded user-facing strings in YouTubeModal with i18n
keys: import/use the translation hook in the YouTubeModal component (e.g., const
{ t } = useTranslation()) and change setError("Invalid YouTube URL") to
setError(t('youtube.invalidUrl')), the "Paste" label to t('youtube.paste'), and
the "Submit" label to t('youtube.submit'); add the corresponding keys and
translations to the app locale files and update any tests or snapshots that
assert these exact strings.
In `@apps/mobile-app/src/hooks/useMedia.ts`:
- Around line 99-101: Add a brief inline comment in useMedia.ts next to the
refetchOnMount: "always" override (where socketFirstQueryOptions is spread)
explaining why it intentionally overrides the socket-first default
(refetchOnMount: false) — e.g., to ensure fresh artifact data on hook remounts
after navigation — so future maintainers understand the rationale when looking
at socketFirstQueryOptions and the useMedia hook.
In `@apps/mobile-app/src/hooks/useSocketSync.ts`:
- Around line 166-173: The refreshArtifacts callback in useSocketSync.ts is
causing excessive artifact refetches on rapid socket events; wrap or replace it
with a per-mediaId coalescing guard (e.g., a Map tracking debounce timers or
in-flight flags keyed by mediaId) so repeated calls within a short window
debounce into a single
queryClient.invalidateQueries(mediaKeys.artifacts(mediaId)) call; update the
refreshArtifacts implementation and any callers (e.g., handlers for
media_chunk_ready/media_batch_ready) to use this coalescing function and choose
a short debounce (e.g., 300–1000ms) or clear the flag when the invalidate
completes to avoid hammering the artifacts endpoint.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37f3a867-d859-44e7-b7fb-a2c47e5fb036
📒 Files selected for processing (18)
.github/copilot-instructions.md.github/instructions/ai-engine.instructions.mdapps/ai-engine/src/async_pipeline.pyapps/ai-engine/src/config.pyapps/ai-engine/src/minio_client.pyapps/ai-engine/tests/test_first_batch_streaming.pyapps/ai-engine/tests/test_minio_public_presign.pyapps/backend-api/src/modules/media/dto/request.dto.tsapps/backend-api/src/modules/media/dto/response.dto.tsapps/backend-api/src/modules/media/media.service.tsapps/backend-api/src/modules/media/yt-dlp.service.tsapps/mobile-app/src/app/(app)/player.tsxapps/mobile-app/src/components/media/UploadModals.tsxapps/mobile-app/src/components/media/YouTubeModal.tsxapps/mobile-app/src/hooks/useMedia.tsapps/mobile-app/src/hooks/usePlayerSubtitles.tsapps/mobile-app/src/hooks/useSocketSync.tscheckpoint.md
✅ Files skipped from review due to trivial changes (4)
- .github/instructions/ai-engine.instructions.md
- apps/backend-api/src/modules/media/dto/request.dto.ts
- .github/copilot-instructions.md
- apps/mobile-app/src/app/(app)/player.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/backend-api/src/modules/media/dto/response.dto.ts
- apps/backend-api/src/modules/media/media.service.ts
Comment on lines
+30
to
+38
| const fetchArtifactJson = async <T>(url: string): Promise<T> => { | ||
| const response = await fetch(url); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`Failed to fetch subtitles: ${response.status}`); | ||
| } | ||
|
|
||
| return (await response.json()) as T; | ||
| }; |
There was a problem hiding this comment.
Route artifact JSON fetching through the centralized service layer.
fetchArtifactJson performs a direct fetch(url) call with a hardcoded error string. Per coding guidelines, API calls should be routed through centralized Axios services, and hardcoded strings should be avoided.
Consider adding a method like mediaService.fetchArtifactJson<T>(url: string) that wraps the fetch logic and uses app-level error handling.
♻️ Suggested service method
// In media.services.ts
async fetchArtifactJson<T>(url: string): Promise<T> {
const response = await fetch(url);
if (!response.ok) {
throw new ApiError('ARTIFACT_FETCH_FAILED', response.status);
}
return response.json() as Promise<T>;
}As per coding guidelines "apps/mobile-app/src/**/*.{service,ts,tsx}: Route all API calls through centralized Axios services in the mobile app" and "apps/mobile-app/src/**/*.{ts,tsx}: No hardcoded colors or strings in mobile app code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/mobile-app/src/hooks/usePlayerSubtitles.ts` around lines 30 - 38,
fetchArtifactJson is doing a raw fetch with a hardcoded error string; move this
logic into the centralized media service and replace direct calls. Implement
mediaService.fetchArtifactJson<T>(url: string) that uses the app's Axios service
wrapper, throws a typed ApiError (e.g., 'ARTIFACT_FETCH_FAILED') with the
response status on non-OK responses, and returns parsed JSON; then update
usePlayerSubtitles to call mediaService.fetchArtifactJson instead of the local
fetchArtifactJson helper. Ensure no hardcoded strings remain and that error
handling uses the app-level ApiError and logging conventions.
Comment on lines
+215
to
235
| const currentStatus = queryClient.getQueryData<MediaStatusResponse>( | ||
| mediaKeys.status(event.mediaId), | ||
| ); | ||
| const hasNewSourceLanguage = | ||
| Boolean(event.sourceLanguage) && | ||
| event.sourceLanguage !== currentStatus?.sourceLanguage; | ||
|
|
||
| const now = Date.now(); | ||
| const last = lastProgressWrite.current[event.mediaId] ?? 0; | ||
| if (now - last < PROGRESS_THROTTLE_MS) return; // skip | ||
| if (!hasNewSourceLanguage && now - last < PROGRESS_THROTTLE_MS) { | ||
| return; | ||
| } | ||
| lastProgressWrite.current[event.mediaId] = now; | ||
|
|
||
| const patch: Partial<MediaItem> = { | ||
| status: "PROCESSING", | ||
| progress: event.progress, | ||
| currentStep: event.currentStep as MediaPipelineStage, | ||
| estimatedTimeRemaining: event.estimatedTimeRemaining, | ||
| sourceLanguage: event.sourceLanguage ?? currentStatus?.sourceLanguage, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/mobile-app/src/hooks/useSocketSync.ts | sed -n '210,240p'Repository: KMA-Final-Project/project
Length of output: 1476
🏁 Script executed:
rg -n "mergeLivePatch" apps/mobile-app/src/hooks/ -A 10 -B 2Repository: KMA-Final-Project/project
Length of output: 2889
🏁 Script executed:
rg -n "const patch" apps/mobile-app/src/hooks/useSocketSync.ts -A 15 -B 5Repository: KMA-Final-Project/project
Length of output: 4412
🏁 Script executed:
rg -n "interface MediaItem|type MediaItem" apps/mobile-app/src/ -A 15Repository: KMA-Final-Project/project
Length of output: 1161
🏁 Script executed:
rg -n "interface MediaStatusResponse|type MediaStatusResponse" apps/mobile-app/src/ -A 15Repository: KMA-Final-Project/project
Length of output: 1196
🏁 Script executed:
rg -n "sourceLanguage" apps/mobile-app/src/hooks/useSocketSync.ts -B 2 -A 2Repository: KMA-Final-Project/project
Length of output: 739
🏁 Script executed:
rg -n "mediaKeys.status" apps/mobile-app/src/hooks/ -B 3 -A 3Repository: KMA-Final-Project/project
Length of output: 3599
🏁 Script executed:
rg -n "useQuery.*status\|useMediaStatus" apps/mobile-app/src/ -A 8Repository: KMA-Final-Project/project
Length of output: 51
🏁 Script executed:
python3 << 'EOF'
# Verify JavaScript object spread behavior with undefined
import json
# Demonstrate the issue
test_cases = [
{
"current": {"id": "123", "sourceLanguage": "en", "status": "PROCESSING"},
"patch": {"sourceLanguage": None, "progress": 0.5},
"description": "Spreading undefined sourceLanguage overwrites existing value"
}
]
for case in test_cases:
current = case["current"]
patch = case["patch"]
# In JavaScript: const next = { ...current, ...patch }
# This is equivalent to updating current with patch values
next = {**current, **patch}
print(f"Scenario: {case['description']}")
print(f" Current: {current}")
print(f" Patch: {patch}")
print(f" Result: {next}")
print(f" Lost sourceLanguage? {current.get('sourceLanguage') is not None and next.get('sourceLanguage') is None}")
print()
EOFRepository: KMA-Final-Project/project
Length of output: 387
🏁 Script executed:
rg -n "useEffect.*media_progress\|onProgress" apps/mobile-app/src/hooks/useSocketSync.ts -B 2 -A 25 | head -40Repository: KMA-Final-Project/project
Length of output: 51
Conditionally include sourceLanguage in the progress patch to prevent undefined from overwriting existing values.
Line 234 always spreads sourceLanguage into the patch, but if the status query is not yet hydrated, currentStatus will be undefined. When event.sourceLanguage is falsy (typical for progress events), this evaluates to undefined, and because mergeLivePatch() spreads the patch object over cached items, an undefined value can erase an existing sourceLanguage from the list cache.
Proposed fix
+ const sourceLanguage =
+ event.sourceLanguage ?? currentStatus?.sourceLanguage;
const patch: Partial<MediaItem> = {
status: "PROCESSING",
progress: event.progress,
currentStep: event.currentStep as MediaPipelineStage,
estimatedTimeRemaining: event.estimatedTimeRemaining,
- sourceLanguage: event.sourceLanguage ?? currentStatus?.sourceLanguage,
};
+ if (sourceLanguage != null) {
+ patch.sourceLanguage = sourceLanguage;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const currentStatus = queryClient.getQueryData<MediaStatusResponse>( | |
| mediaKeys.status(event.mediaId), | |
| ); | |
| const hasNewSourceLanguage = | |
| Boolean(event.sourceLanguage) && | |
| event.sourceLanguage !== currentStatus?.sourceLanguage; | |
| const now = Date.now(); | |
| const last = lastProgressWrite.current[event.mediaId] ?? 0; | |
| if (now - last < PROGRESS_THROTTLE_MS) return; // skip | |
| if (!hasNewSourceLanguage && now - last < PROGRESS_THROTTLE_MS) { | |
| return; | |
| } | |
| lastProgressWrite.current[event.mediaId] = now; | |
| const patch: Partial<MediaItem> = { | |
| status: "PROCESSING", | |
| progress: event.progress, | |
| currentStep: event.currentStep as MediaPipelineStage, | |
| estimatedTimeRemaining: event.estimatedTimeRemaining, | |
| sourceLanguage: event.sourceLanguage ?? currentStatus?.sourceLanguage, | |
| }; | |
| const currentStatus = queryClient.getQueryData<MediaStatusResponse>( | |
| mediaKeys.status(event.mediaId), | |
| ); | |
| const hasNewSourceLanguage = | |
| Boolean(event.sourceLanguage) && | |
| event.sourceLanguage !== currentStatus?.sourceLanguage; | |
| const now = Date.now(); | |
| const last = lastProgressWrite.current[event.mediaId] ?? 0; | |
| if (!hasNewSourceLanguage && now - last < PROGRESS_THROTTLE_MS) { | |
| return; | |
| } | |
| lastProgressWrite.current[event.mediaId] = now; | |
| const sourceLanguage = | |
| event.sourceLanguage ?? currentStatus?.sourceLanguage; | |
| const patch: Partial<MediaItem> = { | |
| status: "PROCESSING", | |
| progress: event.progress, | |
| currentStep: event.currentStep as MediaPipelineStage, | |
| estimatedTimeRemaining: event.estimatedTimeRemaining, | |
| }; | |
| if (sourceLanguage != null) { | |
| patch.sourceLanguage = sourceLanguage; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/mobile-app/src/hooks/useSocketSync.ts` around lines 215 - 235, The patch
currently always sets sourceLanguage to event.sourceLanguage ??
currentStatus?.sourceLanguage, which can insert undefined and overwrite existing
values; update the logic in useSocketSync.ts where the progress patch is built
(see variables: currentStatus, event, patch) to only include the sourceLanguage
property when event.sourceLanguage is explicitly defined (truthy or non-null) so
mergeLivePatch() won't erase an existing sourceLanguage in the cache; implement
this by conditionally adding sourceLanguage to the Partial<MediaItem> patch
(rather than always setting it to a possibly undefined value).
Comment on lines
+89
to
+92
| │ │ │ ├── test_event_discipline.py # Event ordering + monotonic progress | ||
| │ │ │ ├── test_first_batch_streaming.py # Refinement on/off + first-batch output behavior | ||
| │ │ │ ├── test_prewarm_startup.py # Startup/prewarm behavior | ||
| │ │ │ └── test_streaming_contracts.py # Streaming artifact/output contract |
There was a problem hiding this comment.
Add the new MinIO presign test to this checkpoint tree.
This PR introduces apps/ai-engine/tests/test_minio_public_presign.py, but the tests/ listing here still skips it. Since checkpoint.md is the project source-of-truth, the inventory will be stale immediately after merge unless this section is updated.
📝 Proposed fix
│ │ ├── tests/ # Unit tests (pytest)
│ │ │ ├── test_event_discipline.py # Event ordering + monotonic progress
│ │ │ ├── test_first_batch_streaming.py # Refinement on/off + first-batch output behavior
+│ │ │ ├── test_minio_public_presign.py # Public-endpoint GET presigning
│ │ │ ├── test_prewarm_startup.py # Startup/prewarm behavior
│ │ │ └── test_streaming_contracts.py # Streaming artifact/output contract📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| │ │ │ ├── test_event_discipline.py # Event ordering + monotonic progress | |
| │ │ │ ├── test_first_batch_streaming.py # Refinement on/off + first-batch output behavior | |
| │ │ │ ├── test_prewarm_startup.py # Startup/prewarm behavior | |
| │ │ │ └── test_streaming_contracts.py # Streaming artifact/output contract | |
| │ │ │ ├── test_event_discipline.py # Event ordering + monotonic progress | |
| │ │ │ ├── test_first_batch_streaming.py # Refinement on/off + first-batch output behavior | |
| │ │ │ ├── test_minio_public_presign.py # Public-endpoint GET presigning | |
| │ │ │ ├── test_prewarm_startup.py # Startup/prewarm behavior | |
| │ │ │ └── test_streaming_contracts.py # Streaming artifact/output contract |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@checkpoint.md` around lines 89 - 92, The checkpoint.md tests tree is missing
the new test file test_minio_public_presign.py introduced in the PR; update the
tests listing by adding "test_minio_public_presign.py # MinIO public presign
behavior" into the same tests subtree where test_event_discipline.py,
test_first_batch_streaming.py, test_prewarm_startup.py, and
test_streaming_contracts.py are listed, preserving the existing
formatting/comment style and ordering convention so the checkpoint inventory
reflects the new test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Enhancements
UI Components
Documentation
Chores