ECHO-637 ECHO-649 ECHO-665 audio chunks bug fix, wakelock enhancement, frontend ids for testing frameworks #418
Conversation
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
- static sound file for interruption modal - cosmetic changes
Contributor
WalkthroughThis pull request adds comprehensive testing infrastructure (testId utility and data-testid attributes across 50+ components), refactors wake-lock API (removing mount parameter, adding auto-reacquire), implements iOS audio recording interruption detection with reconnection flow, restructures MODE_COLORS shape in ChatModeSelector, adds interruption analytics events, and updates localization entries accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
LGTM. This PR successfully tackles ECHO-637 (file size/interruption issue on screen dim), ECHO-649 (wake lock enhancement for iOS), and ECHO-665 (test tool IDs) in one coherent refactor. The interruption detection + reconnection flow + wake lock auto-reacquire is solid engineering for handling iOS edge cases. The testId spread pattern across all components is clean and scales well. Watch the MODE_COLORS breaking change in ChatModeSelector — that'll ripple downstream. Audio chunk history + resumeRecording API additions are good hooks for future telemetry/debugging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
echo/frontend/src/components/participant/hooks/useChunkedAudioRecorder.ts (1)
83-210:⚠️ Potential issue | 🟠 MajorInterruption path needs state updates and timer cleanup to prevent stale UI.
When two consecutive suspicious chunks trigger interruption (lines 176–195), you exit early without clearingintervalReforstartRecordingIntervalRef, leaving the timer running and UI showing "recording" even though the recorder is dead. Also,hadInterruptionis a ref, so consumers won't re-render when it flips. The fix needs three parts:
- Add
hadInterruptionstate (line 60) and set ittrueduring interruption,falseon reset (lines 270–274)- Clear both intervals on interruption (same as in
stopRecordingat lines 322–327)- Update
isRecording,isPaused,userPausedto false during interruption- Return a copy from
getChunkHistory()to prevent external mutation (line 380)Proposed changes
Add state near line 60:
const [isRecording, setIsRecording] = useState(false); +const [hadInterruption, setHadInterruption] = useState(false);Lines 176–195, update interruption handler:
if ( suspiciousChunkCountRef.current >= 2 && !hasCalledInterruptionCallbackRef.current ) { hadConsecutiveSuspiciousChunksRef.current = true; hasCalledInterruptionCallbackRef.current = true; if (audioAlertRef.current) { audioAlertRef.current.muted = false; audioAlertRef.current.currentTime = 0; audioAlertRef.current.play().catch((error) => { console.error("Failed to play notification sound:", error); }); } chunkBufferRef.current = []; + setHadInterruption(true); + setIsRecording(false); + setIsPaused(false); + setUserPaused(false); + if (intervalRef.current) clearInterval(intervalRef.current); + if (startRecordingIntervalRef.current) clearInterval(startRecordingIntervalRef.current); onRecordingInterrupted?.(); return; }Lines 270–274, reset state on new session:
suspiciousChunkCountRef.current = 0; hadConsecutiveSuspiciousChunksRef.current = false; chunkHistoryRef.current = []; hasCalledInterruptionCallbackRef.current = false; +setHadInterruption(false);Lines 378–381, return object:
-getChunkHistory: () => chunkHistoryRef.current, -hadInterruption: hadConsecutiveSuspiciousChunksRef.current, +getChunkHistory: () => [...chunkHistoryRef.current], +hadInterruption,echo/frontend/src/components/layout/TabsWithRouter.tsx (1)
25-35: 🧹 Nitpick | 🔵 TrivialMerge
classNameand type rest props withStackProps.
Right now{...rest}can override the required"relative"class, andRecord<string, unknown>drops Mantine prop typing.♻️ Proposed refactor
-import { Box, LoadingOverlay, Stack, Tabs } from "@mantine/core"; +import { Box, LoadingOverlay, Stack, Tabs, type StackProps } from "@mantine/core"; @@ -export const TabsWithRouter = ({ - basePath, - tabs, - loading = false, - ...rest -}: { - basePath: string; - tabs: { value: string; label: string }[]; - loading?: boolean; -} & Record<string, unknown>) => { +export const TabsWithRouter = ({ + basePath, + tabs, + loading = false, + className, + ...rest +}: { + basePath: string; + tabs: { value: string; label: string }[]; + loading?: boolean; +} & StackProps) => { @@ - return ( - <Stack className="relative" {...rest}> + return ( + <Stack + className={"relative" + (className ? " " + className : "")} + {...rest} + >Also applies to: 62-62
echo/frontend/src/locales/it-IT.po (1)
8-8:⚠️ Potential issue | 🟡 MinorLanguage header mismatch - should be
it-IT, noten-US.Yo, the metadata says
Language: en-USbut this is the Italian translation file. Classic copy-pasta from the template. Ship it but fix the header.-"Language: en-US\n" +"Language: it-IT\n"
🤖 Fix all issues with AI agents
In `@echo/frontend/src/components/participant/hooks/useChunkedAudioRecorder.ts`:
- Around line 77-80: The current approach unlocks the audio element during
startRecording (user gesture) but later unmutes and calls .play() from the chunk
handler (audioAlertRef usage) outside a gesture, which fails on iOS Safari;
change the logic so you do NOT unmute+play from the chunk handler—either keep
audioAlertRef muted and play a short silent clip to keep it unlocked, or surface
a UI action that the user must tap (e.g., add an explicit "Dismiss interruption"
button) that calls a new unmuteAndPlay function to unmute audioAlertRef and play
the alert within that user gesture; update startRecording, the chunk handler
that currently calls audioAlertRef.current!.muted = false / .play(), and add the
explicit user gesture handler (unmuteAndPlay) so iOS will allow the alert to
play.
In `@echo/frontend/src/components/participant/ParticipantConversationAudio.tsx`:
- Around line 527-529: The overlayProps object in ParticipantConversationAudio
(the overlayProps={{ color: "#FF9AA2" }} usage) hardcodes a hex color; update it
to use a CSS variable per guidelines (e.g., overlayProps={{ color:
"var(--app-error-overlay)" }} or another appropriate theme variable) so theme
changes propagate. Locate the overlayProps in the ParticipantConversationAudio
component and replace the literal "#FF9AA2" with the chosen CSS variable name,
ensuring any used variable is defined in the global CSS/theme.
- Around line 341-370: In handleReconnect, the catch currently swallows the
thrown error as _error; update the catch to log the actual error (e.g.,
console.error or your logging utility) before calling toast.error and ensure
setIsReconnecting(false) still runs; locate handleReconnect and add a logging
statement referencing the caught error, keeping pendingUploadsRef and
reportInterruption behavior unchanged.
In `@echo/frontend/src/components/project/ProjectDangerZone.tsx`:
- Around line 165-190: The delete flow still uses window.confirm and lacks
inline error feedback; remove any window.confirm calls in the delete flow and
instead make handleDelete call deleteProjectByIdMutation.mutateAsync (await it),
catch errors and set local error state, and only navigate/close the modal (call
closeDeleteModal and proceed to routing) on successful resolution; update the
Modal rendering (the ProjectDangerZone delete modal) to display an Alert
component when that local error state is set (similar to the clone modal Alert
pattern), ensure the Delete button uses deleteProjectByIdMutation.isPending for
loading, and keep testId props and button handlers as-is.
In `@echo/frontend/src/hooks/useVideoWakeLockFallback.ts`:
- Around line 80-94: The retry log in useVideoWakeLockFallback's playVideo
function is inconsistent: the console.log says "retrying in 1s..." but the
setTimeout uses 4000ms; change them to match (preferably set the timeout to
1000ms to match the message) or update the log to "retrying in 4s..." so both
the console.log and the setTimeout delay are consistent; check the video.play
retry block (inside playVideo after video.load()) and update either the message
or the numeric delay accordingly.
In `@echo/frontend/src/hooks/useWakeLock.ts`:
- Around line 47-81: The obtainWakeLock() flow can flip isActive to false when a
re-request fails even though an existing wakeLock.current is still held; add a
fast-path at the top of obtainWakeLock() that returns early if wakeLock.current
exists and wakeLock.current.released === false (setIsActive(true) if needed) to
avoid re-requesting an already-held sentinel, and change the catch block to only
call setIsActive(false) when there is no current sentinel or
wakeLock.current.released === true so the real held state of wakeLock.current is
preserved; reference wakeLock.current, obtainWakeLock(), isActive, and
releaseHandlerRef when implementing these checks.
In `@echo/frontend/src/locales/fr-FR.po`:
- Around line 1636-1639: Translate the remaining English strings in the fr-FR.po
file: replace the English msgstr for "participant.modal.refine.info.title"
(referenced from ParticipantConversationAudio.tsx:442) with an appropriate
French translation (e.g., "Fonctionnalité bientôt disponible"), and locate and
localize the other English entries referenced in the comment ranges (lines
1765-1773, 2993-2996, 3720-3727) so all msgid values in
echo/frontend/src/locales/fr-FR.po have French msgstr equivalents; ensure tone
matches the suggested informal French and keep msgid keys unchanged.
In `@echo/frontend/src/locales/it-IT.po`:
- Around line 461-469: Several webhook UI strings in the Italian PO file are
left in English; update the msgstr values to proper Italian translations for
each msgid coming from WebhookSettingsCard.tsx (e.g., the steps "1. You provide
a URL...", "2. When a conversation event happens...", "3. Your system receives
the data...") and other occurrences the reviewer listed. Locate the msgid
entries in the it-IT.po and replace the English msgstr with accurate Italian
equivalents (keeping original msgid text intact), ensuring plural forms and
punctuation are preserved; after editing, run the i18n extractor/compile step
used by the project to validate the PO file and confirm the translated strings
appear in the WebhookSettingsCard UI.
echo/frontend/src/components/participant/hooks/useChunkedAudioRecorder.ts
Show resolved
Hide resolved
Comment on lines
+341
to
+370
| const handleReconnect = async () => { | ||
| setIsReconnecting(true); | ||
| try { | ||
| // Small delay to ensure final chunk's upload promise is added to the array | ||
| await new Promise((resolve) => setTimeout(resolve, 100)); | ||
|
|
||
| // Wait for all pending uploads to complete (with timeout) | ||
| if (pendingUploadsRef.current.length > 0) { | ||
| const timeoutPromise = new Promise<"timeout">((resolve) => | ||
| setTimeout(() => resolve("timeout"), 30000), | ||
| ); | ||
|
|
||
| const result = await Promise.race([ | ||
| Promise.allSettled(pendingUploadsRef.current).then(() => "done"), | ||
| timeoutPromise, | ||
| ]); | ||
|
|
||
| if (result === "timeout") { | ||
| console.warn("Upload wait timeout reached, proceeding anyway"); | ||
| } | ||
| } | ||
|
|
||
| reportInterruption(); | ||
|
|
||
| window.location.reload(); | ||
| } catch (_error) { | ||
| toast.error(t`Failed to reconnect. Please try reloading the page.`); | ||
| setIsReconnecting(false); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
handleReconnect upload wait pattern looks good, minor error handling nit.
The upload wait with 30s timeout matches the handleConfirmFinish pattern - consistency is 🔥.
One small thing: Line 366 uses _error (intentionally unused), but if something in the try block throws before window.location.reload(), the error is silently swallowed. Consider logging it:
♻️ Optional: Log the error before showing toast
} catch (_error) {
+ console.error("Reconnect failed:", _error);
toast.error(t`Failed to reconnect. Please try reloading the page.`);
setIsReconnecting(false);
}📝 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 handleReconnect = async () => { | |
| setIsReconnecting(true); | |
| try { | |
| // Small delay to ensure final chunk's upload promise is added to the array | |
| await new Promise((resolve) => setTimeout(resolve, 100)); | |
| // Wait for all pending uploads to complete (with timeout) | |
| if (pendingUploadsRef.current.length > 0) { | |
| const timeoutPromise = new Promise<"timeout">((resolve) => | |
| setTimeout(() => resolve("timeout"), 30000), | |
| ); | |
| const result = await Promise.race([ | |
| Promise.allSettled(pendingUploadsRef.current).then(() => "done"), | |
| timeoutPromise, | |
| ]); | |
| if (result === "timeout") { | |
| console.warn("Upload wait timeout reached, proceeding anyway"); | |
| } | |
| } | |
| reportInterruption(); | |
| window.location.reload(); | |
| } catch (_error) { | |
| toast.error(t`Failed to reconnect. Please try reloading the page.`); | |
| setIsReconnecting(false); | |
| } | |
| }; | |
| const handleReconnect = async () => { | |
| setIsReconnecting(true); | |
| try { | |
| // Small delay to ensure final chunk's upload promise is added to the array | |
| await new Promise((resolve) => setTimeout(resolve, 100)); | |
| // Wait for all pending uploads to complete (with timeout) | |
| if (pendingUploadsRef.current.length > 0) { | |
| const timeoutPromise = new Promise<"timeout">((resolve) => | |
| setTimeout(() => resolve("timeout"), 30000), | |
| ); | |
| const result = await Promise.race([ | |
| Promise.allSettled(pendingUploadsRef.current).then(() => "done"), | |
| timeoutPromise, | |
| ]); | |
| if (result === "timeout") { | |
| console.warn("Upload wait timeout reached, proceeding anyway"); | |
| } | |
| } | |
| reportInterruption(); | |
| window.location.reload(); | |
| } catch (_error) { | |
| console.error("Reconnect failed:", _error); | |
| toast.error(t`Failed to reconnect. Please try reloading the page.`); | |
| setIsReconnecting(false); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@echo/frontend/src/components/participant/ParticipantConversationAudio.tsx`
around lines 341 - 370, In handleReconnect, the catch currently swallows the
thrown error as _error; update the catch to log the actual error (e.g.,
console.error or your logging utility) before calling toast.error and ensure
setIsReconnecting(false) still runs; locate handleReconnect and add a logging
statement referencing the caught error, keeping pendingUploadsRef and
reportInterruption behavior unchanged.
Comment on lines
+527
to
+529
| overlayProps={{ | ||
| color: "#FF9AA2", | ||
| }} |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Hardcoded hex color in overlay - use CSS variable per guidelines.
Yo, overlayProps={{ color: "#FF9AA2" }} is a hardcoded hex value. Per coding guidelines, should use CSS variables like var(--app-error-overlay) or similar to ensure theme changes propagate. Not critical since this is an alertdialog overlay, but worth noting. As per coding guidelines: "Use CSS variables var(--app-background) and var(--app-text) instead of hardcoded colors."
🤖 Prompt for AI Agents
In `@echo/frontend/src/components/participant/ParticipantConversationAudio.tsx`
around lines 527 - 529, The overlayProps object in ParticipantConversationAudio
(the overlayProps={{ color: "#FF9AA2" }} usage) hardcodes a hex color; update it
to use a CSS variable per guidelines (e.g., overlayProps={{ color:
"var(--app-error-overlay)" }} or another appropriate theme variable) so theme
changes propagate. Locate the overlayProps in the ParticipantConversationAudio
component and replace the literal "#FF9AA2" with the chosen CSS variable name,
ensuring any used variable is defined in the global CSS/theme.
Comment on lines
+165
to
+190
| <Modal | ||
| opened={isDeleteModalOpen} | ||
| onClose={closeDeleteModal} | ||
| title={<Trans>Delete Project</Trans>} | ||
| {...testId("project-delete-modal")} | ||
| > | ||
| <Stack gap="md"> | ||
| <Text size="sm"> | ||
| <Trans> | ||
| Are you sure you want to delete this project? This action cannot | ||
| be undone. | ||
| </Trans> | ||
| </Text> | ||
| </Stack> | ||
| <Group justify="flex-end"> | ||
| <Button variant="default" onClick={closeDeleteModal}> | ||
| <Trans>Cancel</Trans> | ||
| </Button> | ||
| <Button | ||
| onClick={handleDelete} | ||
| loading={deleteProjectByIdMutation.isPending} | ||
| {...testId("project-delete-confirm-button")} | ||
| > | ||
| <Trans>Delete Project</Trans> | ||
| </Button> | ||
| </Group> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate and examine ProjectDangerZone.tsx
fd -t f "ProjectDangerZone.tsx" --exec cat -n {} \;Repository: Dembrane/echo
Length of output: 6699
Remove the window.confirm and add inline error handling to match the clone modal pattern.
The delete handler still triggers window.confirm (line 67), creating redundant confirms and breaking automation. The modal also lacks error feedback—the clone modal in the same file already shows the correct approach with an Alert component (lines 132-142).
Use mutateAsync and await the mutation before navigating, so deletion failures surface inline instead of silently breaking the UX.
Proposed approach
const handleDelete = async () => {
try {
analytics.trackEvent(events.DELETE_PROJECT);
} catch (error) {
console.warn("Analytics tracking failed:", error);
}
try {
await deleteProjectByIdMutation.mutateAsync(project.id);
navigate("/projects");
} catch (_error) {
// toast handled in mutation hook
}
};<Stack gap="md">
<Text size="sm">
<Trans>
Are you sure you want to delete this project? This action cannot
be undone.
</Trans>
</Text>
+ {!deleteProjectByIdMutation.isPending &&
+ deleteProjectByIdMutation.error && (
+ <Alert title={t`Error deleting project`} color="red" variant="light">
+ <Trans>
+ There was an error deleting this project. Please try again.
+ </Trans>
+ </Alert>
+ )}
</Stack>🤖 Prompt for AI Agents
In `@echo/frontend/src/components/project/ProjectDangerZone.tsx` around lines 165
- 190, The delete flow still uses window.confirm and lacks inline error
feedback; remove any window.confirm calls in the delete flow and instead make
handleDelete call deleteProjectByIdMutation.mutateAsync (await it), catch errors
and set local error state, and only navigate/close the modal (call
closeDeleteModal and proceed to routing) on successful resolution; update the
Modal rendering (the ProjectDangerZone delete modal) to display an Alert
component when that local error state is set (similar to the clone modal Alert
pattern), ensure the Delete button uses deleteProjectByIdMutation.isPending for
loading, and keep testId props and button handlers as-is.
Contributor
Author
There was a problem hiding this comment.
this PR doesnt handle design improvements so that is why this change wont be made in this PR. we'll have another PR to remove all the browser default dialogues etc
Contributor
There was a problem hiding this comment.
@ussaama Totally fair—scoping this PR to test IDs makes sense. LGTM shipping the test hooks as-is and tackling the dialog polish in the follow-up. 🚀
🧠 Learnings used
Learnt from: ussaama
Repo: Dembrane/echo PR: 405
File: echo/frontend/src/components/conversation/ConversationAccordion.tsx:891-954
Timestamp: 2026-01-19T14:03:00.849Z
Learning: In echo/frontend/src/components/conversation/ConversationAccordion.tsx, the select-all error handling (handleSelectAllConfirm) intentionally closes the modal on error and shows only a toast. Errors in this flow only occur when the server doesn't respond, and the design decision is to let users retry by reopening the modal rather than keeping them in an error state within the modal.
Learnt from: CR
Repo: Dembrane/echo PR: 0
File: echo/frontend/AGENTS.md:0-0
Timestamp: 2025-12-05T00:33:33.442Z
Learning: Applies to echo/frontend/**/src/{components,routes}/**/*.tsx : UI mutations should surface inline feedback: pair toasts with contextual Mantine `Alert` components inside modals/forms for errors or warnings
Learnt from: ussaama
Repo: Dembrane/echo PR: 259
File: echo/frontend/src/components/layout/ParticipantLayout.tsx:33-33
Timestamp: 2025-08-08T10:39:31.114Z
Learning: In echo/frontend/src/components/layout/ParticipantLayout.tsx, prefer using simple pathname.includes("start") and pathname.includes("finish") to control the settings button visibility. No need to switch to segment-based matching or add a useEffect to auto-close the modal for these routes, per ussaama’s preference in PR `#259`.
Learnt from: ussaama
Repo: Dembrane/echo PR: 256
File: echo/frontend/src/components/participant/MicrophoneTest.tsx:54-54
Timestamp: 2025-08-06T13:38:30.769Z
Learning: In echo/frontend/src/components/participant/MicrophoneTest.tsx, the useDisclosure hook is used where only the `close` function is needed locally (called in handleConfirmMicChange and handleCancelMicChange), while `opened` and `open` are unused because the modal state is managed by a parent component. The `showSecondModal` state variable is used separately to control content switching within the modal between the main UI and confirmation step.
Learnt from: ussaama
Repo: Dembrane/echo PR: 169
File: echo/frontend/src/components/project/ProjectPortalEditor.tsx:409-464
Timestamp: 2025-05-30T15:38:44.413Z
Learning: Badge-based selectors in ProjectPortalEditor.tsx: Keyboard navigation enhancements for accessibility are considered optional improvements rather than critical issues. The user acknowledges these suggestions but doesn't prioritize them as blockers.
Learnt from: CR
Repo: Dembrane/echo PR: 0
File: echo/frontend/AGENTS.md:0-0
Timestamp: 2025-12-05T00:33:33.442Z
Learning: Applies to echo/frontend/**/src/routes/auth/**/*.tsx : Gentle login/logout flows use `useTransitionCurtain().runTransition()` before navigation—animations expect Directus session mutations to await that promise
Learnt from: ussaama
Repo: Dembrane/echo PR: 205
File: echo/frontend/src/lib/query.ts:1444-1506
Timestamp: 2025-07-10T12:48:20.683Z
Learning: ussaama prefers string concatenation over template literals for simple cases where readability is clearer, even when linting tools suggest template literals. Human readability takes precedence over strict linting rules in straightforward concatenation scenarios.
Comment on lines
+80
to
+94
| video.load(); | ||
|
|
||
| // Play the video | ||
| const playVideo = async () => { | ||
| try { | ||
| await video.play(); | ||
| console.log( | ||
| "[VideoWakeLockFallback] Silent 1-pixel video playing as fallback for wakelock", | ||
| ); | ||
| await video.play().catch(() => { | ||
| console.log( | ||
| "[VideoWakeLockFallback] First play failed, retrying in 1s...", | ||
| ); | ||
| setTimeout(() => { | ||
| video.play().catch(() => { | ||
| console.warn("[VideoWakeLockFallback] Retry also failed"); | ||
| }); | ||
| }, 4000); | ||
| }); |
Contributor
There was a problem hiding this comment.
Fix retry delay/log mismatch to avoid debugging drift.
Line 85-93 logs “1s” but waits 4s; align the message or the delay.
🛠️ Minimal fix (message only)
- console.log(
- "[VideoWakeLockFallback] First play failed, retrying in 1s...",
- );
+ console.log(
+ "[VideoWakeLockFallback] First play failed, retrying in 4s...",
+ );📝 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
| video.load(); | |
| // Play the video | |
| const playVideo = async () => { | |
| try { | |
| await video.play(); | |
| console.log( | |
| "[VideoWakeLockFallback] Silent 1-pixel video playing as fallback for wakelock", | |
| ); | |
| await video.play().catch(() => { | |
| console.log( | |
| "[VideoWakeLockFallback] First play failed, retrying in 1s...", | |
| ); | |
| setTimeout(() => { | |
| video.play().catch(() => { | |
| console.warn("[VideoWakeLockFallback] Retry also failed"); | |
| }); | |
| }, 4000); | |
| }); | |
| video.load(); | |
| // Play the video | |
| const playVideo = async () => { | |
| try { | |
| await video.play().catch(() => { | |
| console.log( | |
| "[VideoWakeLockFallback] First play failed, retrying in 4s...", | |
| ); | |
| setTimeout(() => { | |
| video.play().catch(() => { | |
| console.warn("[VideoWakeLockFallback] Retry also failed"); | |
| }); | |
| }, 4000); | |
| }); |
🤖 Prompt for AI Agents
In `@echo/frontend/src/hooks/useVideoWakeLockFallback.ts` around lines 80 - 94,
The retry log in useVideoWakeLockFallback's playVideo function is inconsistent:
the console.log says "retrying in 1s..." but the setTimeout uses 4000ms; change
them to match (preferably set the timeout to 1000ms to match the message) or
update the log to "retrying in 4s..." so both the console.log and the setTimeout
delay are consistent; check the video.play retry block (inside playVideo after
video.load()) and update either the message or the numeric delay accordingly.
Contributor
Author
There was a problem hiding this comment.
no issue ignore for now
Contributor
There was a problem hiding this comment.
@ussaama LGTM. Noted for now. 🚀
🧠 Learnings used
Learnt from: ussaama
Repo: Dembrane/echo PR: 205
File: echo/frontend/src/lib/query.ts:1444-1506
Timestamp: 2025-07-10T12:48:20.683Z
Learning: ussaama prefers string concatenation over template literals for simple cases where readability is clearer, even when linting tools suggest template literals. Human readability takes precedence over strict linting rules in straightforward concatenation scenarios.
Comment on lines
+47
to
81
| try { | ||
| // Request new wakelock FIRST to avoid gap where screen can sleep | ||
| const newWakeLock = await navigator.wakeLock.request("screen"); | ||
|
|
||
| const handleRelease = () => { | ||
| setIsActive(false); | ||
| }; | ||
| releaseHandlerRef.current = handleRelease; | ||
| wakelock.addEventListener("release", handleRelease); | ||
| // THEN release old one (if exists) | ||
| if (wakeLock.current) { | ||
| if (releaseHandlerRef.current) { | ||
| wakeLock.current.removeEventListener( | ||
| "release", | ||
| releaseHandlerRef.current, | ||
| ); | ||
| releaseHandlerRef.current = null; | ||
| } | ||
| } catch (_err) { | ||
| setIsActive(false); | ||
| await wakeLock.current.release(); | ||
| } | ||
| } else { | ||
| setIsSupported(false); | ||
|
|
||
| // Now assign the new one | ||
| wakeLock.current = newWakeLock; | ||
| setIsActive(true); | ||
| console.log("[WakeLock] Acquired"); | ||
|
|
||
| const handleRelease = () => { | ||
| setIsActive(false); | ||
| console.log("[WakeLock] Released by system"); | ||
| }; | ||
| releaseHandlerRef.current = handleRelease; | ||
| newWakeLock.addEventListener("release", handleRelease); | ||
| } catch (err) { | ||
| // Wake lock request can fail due to: | ||
| // - Battery saver mode | ||
| // - System policy | ||
| // - No user interaction yet | ||
| console.warn("[WakeLock] Failed to acquire:", err); | ||
| setIsActive(false); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "useWakeLock.ts" --type fRepository: Dembrane/echo
Length of output: 95
🏁 Script executed:
cat -n echo/frontend/src/hooks/useWakeLock.tsRepository: Dembrane/echo
Length of output: 4704
Guard against state drift when re‑acquiring an already-held wake lock.
If obtainWakeLock() is called while a lock is already active and the re-request fails, isActive flips to false even though the prior sentinel is still held. Add a fast-path check to skip re-requesting if a lock already exists and hasn't been released, and on error, preserve the actual state of the current lock.
🛠️ Suggested fix
const obtainWakeLock = async () => {
+ if (wakeLock.current && !wakeLock.current.released) {
+ setIsActive(true);
+ return;
+ }
// Only attempt if supported
if (!("wakeLock" in navigator)) {
console.warn("[WakeLock] Cannot obtain - API not supported");
setIsSupported(false);
setIsActive(false);
return;
}
@@
} catch (err) {
// Wake lock request can fail due to:
// - Battery saver mode
// - System policy
// - No user interaction yet
console.warn("[WakeLock] Failed to acquire:", err);
- setIsActive(false);
+ setIsActive(!!wakeLock.current && !wakeLock.current.released);
}
};🤖 Prompt for AI Agents
In `@echo/frontend/src/hooks/useWakeLock.ts` around lines 47 - 81, The
obtainWakeLock() flow can flip isActive to false when a re-request fails even
though an existing wakeLock.current is still held; add a fast-path at the top of
obtainWakeLock() that returns early if wakeLock.current exists and
wakeLock.current.released === false (setIsActive(true) if needed) to avoid
re-requesting an already-held sentinel, and change the catch block to only call
setIsActive(false) when there is no current sentinel or
wakeLock.current.released === true so the real held state of wakeLock.current is
preserved; reference wakeLock.current, obtainWakeLock(), isActive, and
releaseHandlerRef when implementing these checks.
Contributor
Author
There was a problem hiding this comment.
not a critical issue and also this already works fine
Contributor
There was a problem hiding this comment.
@ussaama LGTM. Fair call – if it's working solid in practice, no need to over-engineer the edge case. Appreciate the heads up! 🚀
🧠 Learnings used
Learnt from: ussaama
Repo: Dembrane/echo PR: 205
File: echo/frontend/src/lib/query.ts:1444-1506
Timestamp: 2025-07-10T12:48:20.683Z
Learning: ussaama prefers string concatenation over template literals for simple cases where readability is clearer, even when linting tools suggest template literals. Human readability takes precedence over strict linting rules in straightforward concatenation scenarios.
Comment on lines
+1636
to
1639
| #: src/components/participant/ParticipantConversationAudio.tsx:442 | ||
| msgid "participant.modal.refine.info.title" | ||
| msgstr "Feature Bientôt disponible" | ||
|
|
Contributor
There was a problem hiding this comment.
Localize remaining English strings in fr-FR.
Line 1636-1639, Line 1765-1773, Line 2993-2996, and Line 3720-3727 still ship English text.
🌍 Suggested French translations (informal tone)
msgid "participant.modal.refine.info.title"
-msgstr "Feature Bientôt disponible"
+msgstr "Fonction bientôt disponible"
msgid "Get an immediate reply from Dembrane to help you deepen the conversation."
-msgstr "Get an immediate reply from Dembrane to help you deepen the conversation."
+msgstr "Obtiens une réponse immédiate de Dembrane pour approfondir la conversation."
msgid "participant.refine.go.deeper.description"
-msgstr "Get an immediate reply from Dembrane to help you deepen the conversation."
+msgstr "Obtiens une réponse immédiate de Dembrane pour approfondir la conversation."
msgid "participant.button.refine"
-msgstr "Refine"
+msgstr "Affiner"
msgid "Take some time to create an outcome that makes your contribution concrete or get an immediate reply from Dembrane to help you deepen the conversation."
-msgstr "Take some time to create an outcome that makes your contribution concrete or get an immediate reply from Dembrane to help you deepen the conversation."
+msgstr "Prends un moment pour rendre ta contribution concrète, ou obtiens une réponse immédiate de Dembrane pour approfondir la conversation."
msgid "Take some time to create an outcome that makes your contribution concrete."
-msgstr "Take some time to create an outcome that makes your contribution concrete."
+msgstr "Prends un moment pour rendre ta contribution concrète."Also applies to: 1765-1773, 2993-2996, 3720-3727
🤖 Prompt for AI Agents
In `@echo/frontend/src/locales/fr-FR.po` around lines 1636 - 1639, Translate the
remaining English strings in the fr-FR.po file: replace the English msgstr for
"participant.modal.refine.info.title" (referenced from
ParticipantConversationAudio.tsx:442) with an appropriate French translation
(e.g., "Fonctionnalité bientôt disponible"), and locate and localize the other
English entries referenced in the comment ranges (lines 1765-1773, 2993-2996,
3720-3727) so all msgid values in echo/frontend/src/locales/fr-FR.po have French
msgstr equivalents; ensure tone matches the suggested informal French and keep
msgid keys unchanged.
Comment on lines
+461
to
+469
| msgstr "1. You provide a URL where you want to receive notifications" | ||
|
|
||
| #: src/components/project/webhooks/WebhookSettingsCard.tsx:562 | ||
| msgid "2. When a conversation event happens, we automatically send the conversation data to your URL" | ||
| msgstr "" | ||
| msgstr "2. When a conversation event happens, we automatically send the conversation data to your URL" | ||
|
|
||
| #: src/components/project/webhooks/WebhookSettingsCard.tsx:568 | ||
| msgid "3. Your system receives the data and can act on it (e.g., save to a database, send an email, update a spreadsheet)" | ||
| msgstr "" | ||
| msgstr "3. Your system receives the data and can act on it (e.g., save to a database, send an email, update a spreadsheet)" |
Contributor
There was a problem hiding this comment.
Webhook-related strings are English, not Italian.
Massive amount of webhook UI strings (WebhookSettingsCard.tsx references) are using English msgstr in this Italian locale. Looks like these webhook features were added without i18n coverage. Not blocking but flagging for the localization pass.
Also applies to: 547-551, 593-594, 719-720, 887-888, 892-893, 954-955, 1063-1064, 1107-1108, 1267-1268, 1297-1298, 1349-1350, 1397-1398, 1464-1465, 1520-1521, 1574-1575, 1693-1694, 1773-1774, 1899-1900
🤖 Prompt for AI Agents
In `@echo/frontend/src/locales/it-IT.po` around lines 461 - 469, Several webhook
UI strings in the Italian PO file are left in English; update the msgstr values
to proper Italian translations for each msgid coming from
WebhookSettingsCard.tsx (e.g., the steps "1. You provide a URL...", "2. When a
conversation event happens...", "3. Your system receives the data...") and other
occurrences the reviewer listed. Locate the msgid entries in the it-IT.po and
replace the English msgstr with accurate Italian equivalents (keeping original
msgid text intact), ensuring plural forms and punctuation are preserved; after
editing, run the i18n extractor/compile step used by the project to validate the
PO file and confirm the translated strings appear in the WebhookSettingsCard UI.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Bug Fixes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.