Feat/unsaved changes safeguard 1172 - #1222
Conversation
…ified asset serving Signed-off-by: Aditya R. Satapathy <adityaranjanwxd@gmail.com>
…dingReady callback Signed-off-by: Aditya R. Satapathy <adityaranjanwxd@gmail.com>
… and client Signed-off-by: Aditya R. Satapathy <adityaranjanwxd@gmail.com>
…d verify tag callbacks Signed-off-by: Aditya R. Satapathy <adityaranjanwxd@gmail.com>
Signed-off-by: Aditya R. Satapathy <adityaranjanwxd@gmail.com>
Signed-off-by: Aditya R. Satapathy <adityaranjanwxd@gmail.com>
Signed-off-by: Aditya R. Satapathy <adityaranjanwxd@gmail.com>
…ate symbols Signed-off-by: Aditya R. Satapathy <adityaranjanwxd@gmail.com>
…tate during extraction Signed-off-by: Aditya R. Satapathy <adityaranjanwxd@gmail.com>
… files at 13 Signed-off-by: Aditya R. Satapathy <adityaranjanwxd@gmail.com>
…and add draft persistence (itzzavdhesh#1172) Signed-off-by: Aditya R. Satapathy <adityaranjanwxd@gmail.com>
…main VoiceForge composer (itzzavdhesh#1172) Signed-off-by: Aditya R. Satapathy <adityaranjanwxd@gmail.com>
|
@Myparadox-creator is attempting to deploy a commit to the itzzavdhesh's projects Team on Vercel. A member of the Team first needs to authorize it. |
✍️ DCO Sign-off NeededHey @Myparadox-creator! 👋 One or more commits in this PR are missing a Warning
How to fix: For the latest commit: git commit --amend --signoff
git push --force-with-leaseFor multiple commits, replace git rebase --signoff HEAD~N
git push --force-with-leaseThis comment will update automatically after you push. 🤖 VoiceForge Automation · Updates automatically on edits |
github-actions
Bot
added
bug
🛠️ PR Needs UpdatesHey @Myparadox-creator! 👋 A few things need fixing before a mentor can review this PR. Warning
How to fix:
Once fixed, the workflow re-runs automatically and pings the right mentor. 🤖 VoiceForge Automation · Updates automatically on edits |
|
Warning Review limit reached
Next review available in: 50 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe PR adds production Docker and Compose support, SPA serving, draft persistence, unload protection, speech-history tags and analytics, subtitle overlays, recording validation, an application error boundary, and explicit frontend import extensions. It also removes the README About section. ChangesVoiceForge delivery and frontend behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
🧪 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/pages/Call.jsx (1)
208-225: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
activeTextuntil the current playback ends.
useTTS().speak()does not await playback completion, andVideoPreviewshows subtitles only whileaudio.onPlaysetsisSpeakingtotrue. ClearactiveTextfrom a matching playback-ended path instead of the TTS generationfinally; clear it immediately only for a matching generation request that fails before playback starts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/pages/Call.jsx` around lines 208 - 225, Update the speak flow around setActiveText and useTTS().speak() so the finally block no longer clears activeText after generation returns. Clear activeText from the matching playback-ended handler, and only clear it immediately when the matching generation request fails before playback begins; ensure stale or unrelated playback events cannot clear newer active text.
🧹 Nitpick comments (3)
client/src/hooks/useUnsavedChanges.test.js (1)
10-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest does not exercise the real hook implementation.
This test defines its own copy of
handleBeforeUnloadinstead of invoking the handler thatuseUnsavedChangesregisters. If the real handler inuseUnsavedChanges.jschanges or breaks, this test still passes, because it only checks the local duplicate.Render a component that calls the real hook, and capture the listener passed to
window.addEventListener("beforeunload", ...)for testing.♻️ Proposed fix to test the real hook
-import { describe, it, expect, vi } from "vitest"; +import { describe, it, expect, vi } from "vitest"; +import { render } from "`@testing-library/react`"; +import { useUnsavedChanges } from "./useUnsavedChanges.js"; + +function TestComponent({ condition }) { + useUnsavedChanges(condition); + return null; +} describe("useUnsavedChanges module", () => { it("exports useUnsavedChanges function", async () => { const mod = await import("./useUnsavedChanges.js"); expect(typeof mod.useUnsavedChanges).toBe("function"); expect(typeof mod.default).toBe("function"); }); it("handles beforeunload event and prevents default", () => { - const event = { - preventDefault: vi.fn(), - returnValue: undefined, - }; - - const handleBeforeUnload = (evt) => { - evt.preventDefault(); - evt.returnValue = ""; - return ""; - }; - - handleBeforeUnload(event); - - expect(event.preventDefault).toHaveBeenCalled(); - expect(event.returnValue).toBe(""); + const addSpy = vi.spyOn(window, "addEventListener"); + render(<TestComponent condition={true} />); + const call = addSpy.mock.calls.find(([type]) => type === "beforeunload"); + const handler = call?.[1]; + const event = { preventDefault: vi.fn(), returnValue: undefined }; + handler(event); + expect(event.preventDefault).toHaveBeenCalled(); + expect(event.returnValue).toBe(""); }); });Verify whether
@testing-library/react(or an equivalent) is already used in this project's test suite before applying this pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/hooks/useUnsavedChanges.test.js` around lines 10 - 27, Replace the locally defined handleBeforeUnload test logic in useUnsavedChanges.test.js with a component that invokes the real useUnsavedChanges hook. Capture the beforeunload listener registered through window.addEventListener, using the existing `@testing-library/react` or equivalent test utilities if available, then invoke that captured handler and retain the preventDefault and returnValue assertions.client/src/pages/Onboarding.jsx (1)
222-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded
10duplicatesMIN_DURATIONfromVoiceRecorder.jsx.
isValidhere is computed against a literal10in two places, and the Clone button'sdisabledcondition at Line 408 does the same.VoiceRecorder.jsxuses aMIN_DURATIONconstant for this threshold. If that constant changes, this file will silently disagree with it.Export
MIN_DURATIONfrom a shared module and import it in both files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/pages/Onboarding.jsx` around lines 222 - 236, Replace the hardcoded duration threshold in the onboarding validation logic and Clone button disabled condition with a shared MIN_DURATION constant. Export MIN_DURATION from a shared module, then import and use it in both Onboarding.jsx and VoiceRecorder.jsx so all duration checks remain consistent when the threshold changes.client/src/components/VoiceForge.jsx (1)
8-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared draft-persistence hook. Both files independently implement the same three-part pattern: restore text from
sessionStoragein auseStateinitializer, sync it back with an effect, and feed the trimmed result intouseUnsavedChanges. Both also manually callsessionStorage.removeItemafter clearing state, which is redundant with their own sync effect. A shared hook (e.g.,useDraftText(key)returning[value, setValue, clearDraft]that internally wrapsuseUnsavedChanges) removes the duplication and the redundant manual cleanup calls.
client/src/components/VoiceForge.jsx#L8-L47: replace theinputTextinit/effect/useUnsavedChangesblock with the shared hook, and drop the manualsessionStorage.removeItemat Lines 165-167 in favor of the hook'sclearDraft.client/src/components/TextToSpeech.jsx#L5-L92: replace thetextinit/effect/useUnsavedChangesblock with the shared hook, and drop the manualsessionStorage.removeItemat Lines 140-142 in favor of the hook'sclearDraft.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/components/VoiceForge.jsx` around lines 8 - 47, Extract a shared useDraftText hook that restores and synchronizes sessionStorage, invokes useUnsavedChanges with the trimmed value, and returns the draft value, setter, and clearDraft function. In client/src/components/VoiceForge.jsx lines 8-47, replace the local inputText persistence block with this hook and use clearDraft instead of the manual sessionStorage.removeItem at lines 165-167; apply the same replacement in client/src/components/TextToSpeech.jsx lines 5-92 and use clearDraft instead of its manual removal at lines 140-142.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.dockerignore:
- Around line 16-18: Update the .dockerignore environment-file patterns to
exclude all .env.* files, including production and development variants, while
explicitly unignoring only the committed example templates. Ensure these rules
prevent environment files from reaching either the Dockerfile builder copy or
the copied server directory.
In `@client/src/components/VoiceForge.jsx`:
- Around line 8-47: In VoiceForge’s inputText initializer, cap the
sessionStorage draft to MAX_CHARS when restoring it, and update handleSpeak to
reject input whose length exceeds MAX_CHARS before submission. Preserve the
existing textarea limit and ensure the Speak & Save action cannot submit
overlong restored drafts.
In `@client/src/pages/Onboarding.jsx`:
- Around line 207-241: Update the clone-success flow in handleClone to clear
recording after the submitted clip is successfully sent, then remove the
successProfile condition from useUnsavedChanges so protection depends directly
on an existing recording blob or active clone operation. Keep successProfile for
its other UI/state purposes if still needed.
---
Outside diff comments:
In `@client/src/pages/Call.jsx`:
- Around line 208-225: Update the speak flow around setActiveText and
useTTS().speak() so the finally block no longer clears activeText after
generation returns. Clear activeText from the matching playback-ended handler,
and only clear it immediately when the matching generation request fails before
playback begins; ensure stale or unrelated playback events cannot clear newer
active text.
---
Nitpick comments:
In `@client/src/components/VoiceForge.jsx`:
- Around line 8-47: Extract a shared useDraftText hook that restores and
synchronizes sessionStorage, invokes useUnsavedChanges with the trimmed value,
and returns the draft value, setter, and clearDraft function. In
client/src/components/VoiceForge.jsx lines 8-47, replace the local inputText
persistence block with this hook and use clearDraft instead of the manual
sessionStorage.removeItem at lines 165-167; apply the same replacement in
client/src/components/TextToSpeech.jsx lines 5-92 and use clearDraft instead of
its manual removal at lines 140-142.
In `@client/src/hooks/useUnsavedChanges.test.js`:
- Around line 10-27: Replace the locally defined handleBeforeUnload test logic
in useUnsavedChanges.test.js with a component that invokes the real
useUnsavedChanges hook. Capture the beforeunload listener registered through
window.addEventListener, using the existing `@testing-library/react` or equivalent
test utilities if available, then invoke that captured handler and retain the
preventDefault and returnValue assertions.
In `@client/src/pages/Onboarding.jsx`:
- Around line 222-236: Replace the hardcoded duration threshold in the
onboarding validation logic and Clone button disabled condition with a shared
MIN_DURATION constant. Export MIN_DURATION from a shared module, then import and
use it in both Onboarding.jsx and VoiceRecorder.jsx so all duration checks
remain consistent when the threshold changes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f900dd54-bf96-471d-a2a4-b23cd339d5dd
📒 Files selected for processing (17)
.dockerignoreDockerfileREADME.mdclient/src/App.jsxclient/src/components/SpeechHistory.jsxclient/src/components/TextToSpeech.jsxclient/src/components/VideoPreview.jsxclient/src/components/VoiceForge.jsxclient/src/components/VoiceRecorder.jsxclient/src/hooks/useSpeechHistory.jsclient/src/hooks/useUnsavedChanges.jsclient/src/hooks/useUnsavedChanges.test.jsclient/src/main.jsxclient/src/pages/Call.jsxclient/src/pages/Onboarding.jsxdocker-compose.ymlserver/index.js
💤 Files with no reviewable changes (1)
- README.md
| .env | ||
| .env.local | ||
| .env.*.local |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Exclude all environment files from the Docker build context.
These patterns do not exclude .env.production or .env.development. Dockerfile Line 17 copies those files into the builder stage. A server-scoped file can also reach the runtime image through the copied server directory.
Ignore .env.* and explicitly unignore only committed example templates.
Proposed fix
.env
-.env.local
-.env.*.local
+.env.*
+!.env.example📝 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.
| .env | |
| .env.local | |
| .env.*.local | |
| .env | |
| .env.* | |
| !.env.example |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.dockerignore around lines 16 - 18, Update the .dockerignore
environment-file patterns to exclude all .env.* files, including production and
development variants, while explicitly unignoring only the committed example
templates. Ensure these rules prevent environment files from reaching either the
Dockerfile builder copy or the copied server directory.
| import { VoiceQuickSettings } from "./VoiceQuickSettings.jsx"; | ||
| import { FavoriteMessages } from "./FavoriteMessages.jsx"; | ||
| import { QuickReplies } from "./QuickReplies.jsx"; | ||
| import { SpeechHistory } from "./SpeechHistory.jsx"; | ||
| import { ToastContainer, useToast } from "./useToast.jsx"; | ||
| import { useSpeechHistory } from "../hooks/useSpeechHistory.js"; | ||
| import { LanguageSelector } from "./LanguageSelector.jsx"; | ||
| import { loadLanguage, persistLanguage } from "../utils/languages.js"; | ||
| import useTTS from "../hooks/useTTS.js"; | ||
| import { getActiveVoiceProfile } from "../hooks/useVoiceClone.js"; | ||
| import { saveAudioBlob, getAudioBlob } from "../utils/db.js"; | ||
| import { useUnsavedChanges } from "../hooks/useUnsavedChanges.js"; | ||
|
|
||
| const MAX_CHARS = 300; | ||
| const DRAFT_KEY = "voiceforge_composer_draft_text"; | ||
|
|
||
| export default function VoiceForge() { | ||
| const [inputText, setInputText] = useState(""); | ||
| const [inputText, setInputText] = useState(() => { | ||
| try { | ||
| return sessionStorage.getItem(DRAFT_KEY) || ""; | ||
| } catch { | ||
| return ""; | ||
| } | ||
| }); | ||
| const [isSpeaking, setIsSpeaking] = useState(false); | ||
| const [language, setLanguage] = useState(loadLanguage); | ||
| const [historyOpen, setHistoryOpen] = useState(false); | ||
| const drawerRef = useRef(null); | ||
| const historyToggleRef = useRef(null); | ||
|
|
||
| useEffect(() => { | ||
| try { | ||
| if (inputText.length > 0) { | ||
| sessionStorage.setItem(DRAFT_KEY, inputText); | ||
| } else { | ||
| sessionStorage.removeItem(DRAFT_KEY); | ||
| } | ||
| } catch {} | ||
| }, [inputText]); | ||
|
|
||
| useUnsavedChanges(inputText.trim().length > 0); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restored draft can exceed MAX_CHARS with no submission guard.
The initializer restores inputText from sessionStorage without slicing to MAX_CHARS. Unlike the textarea's onChange handler at Line 364, this restoration path bypasses the length cap. handleSpeak (Lines 154-168) and the Speak & Save button's disabled condition also do not reject text longer than MAX_CHARS. TextToSpeech.jsx enforces this same limit at both the initializer level and inside submit(); this file does not.
Cap the restored value at read time, and add a length guard to handleSpeak to match TextToSpeech.jsx's behavior.
🐛 Proposed fix
const [inputText, setInputText] = useState(() => {
try {
- return sessionStorage.getItem(DRAFT_KEY) || "";
+ return (sessionStorage.getItem(DRAFT_KEY) || "").slice(0, MAX_CHARS);
} catch {
return "";
}
}); const handleSpeak = useCallback(() => {
const text = inputText.trim();
- if (!text) {
+ if (!text || text.length > MAX_CHARS) {
showToast("Please type a message first", "error");
textareaRef.current?.focus();
return;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/src/components/VoiceForge.jsx` around lines 8 - 47, In VoiceForge’s
inputText initializer, cap the sessionStorage draft to MAX_CHARS when restoring
it, and update handleSpeak to reject input whose length exceeds MAX_CHARS before
submission. Preserve the existing textarea limit and ensure the Speak & Save
action cannot submit overlong restored drafts.
| useUnsavedChanges((Boolean(recording?.blob) || isCloning) && !successProfile); | ||
|
|
||
| const handleRecordingReady = React.useCallback((blobArg, metaArg) => { | ||
| if (!blobArg) { | ||
| setRecording(null); | ||
| return; | ||
| } | ||
| let blob = blobArg instanceof Blob ? blobArg : blobArg?.blob; | ||
| if (!blob && !(blobArg instanceof Blob)) { | ||
| setRecording(null); | ||
| return; | ||
| } | ||
| let duration = 0; | ||
| let isValid = false; | ||
|
|
||
| if (typeof metaArg === "number") { | ||
| duration = metaArg; | ||
| isValid = duration >= 10; | ||
| } else if (metaArg && typeof metaArg === "object") { | ||
| duration = metaArg.duration || 0; | ||
| isValid = metaArg.isValid !== undefined ? metaArg.isValid : duration >= 10; | ||
| } else if (blobArg && typeof blobArg === "object" && !(blobArg instanceof Blob)) { | ||
| blob = blobArg.blob; | ||
| duration = blobArg.duration || 0; | ||
| isValid = blobArg.isValid !== undefined ? blobArg.isValid : duration >= 10; | ||
| } | ||
|
|
||
| if (!isValid && duration >= 10) { | ||
| isValid = true; | ||
| } | ||
|
|
||
| setRecording({ blob: blob || blobArg, duration, isValid }); | ||
| }, []); | ||
|
|
||
| const recordingDuration = recording?.duration || 0; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stale successProfile can suppress unload protection for a new recording.
successProfile is set once by handleClone and never reset. If a user returns to Step 1 after a successful clone and records a new reference clip, recording.blob becomes truthy again, but useUnsavedChanges still evaluates to false because !successProfile is permanently false. The new, genuinely unsaved recording loses beforeunload protection.
Clear recording after a successful clone (it has already been sent to the server) instead of gating on successProfile.
🐛 Proposed fix
useUnsavedChanges((Boolean(recording?.blob) || isCloning) && !successProfile); async function handleClone() {
if (!recording || !recording.isValid) return;
const profile = await cloneVoice(recording.blob, voiceName);
setSuccessProfile(profile);
+ setRecording(null);
}With this change, useUnsavedChanges can drop the && !successProfile term entirely, since recording is only truthy while there's an actual unsaved clip:
- useUnsavedChanges((Boolean(recording?.blob) || isCloning) && !successProfile);
+ useUnsavedChanges(Boolean(recording?.blob) || isCloning);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/src/pages/Onboarding.jsx` around lines 207 - 241, Update the
clone-success flow in handleClone to clear recording after the submitted clip is
successfully sent, then remove the successProfile condition from
useUnsavedChanges so protection depends directly on an existing recording blob
or active clone operation. Keep successProfile for its other UI/state purposes
if still needed.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="client/src/components/SpeechHistory.jsx">
<violation number="1" location="client/src/components/SpeechHistory.jsx:26">
P3: A tag literally named `All Tags` cannot be filtered: selecting it is indistinguishable from the all-tags sentinel. Represent the unfiltered state separately (for example, `null`) so this valid user tag remains selectable.</violation>
<violation number="2" location="client/src/components/SpeechHistory.jsx:55">
P3: Top Phrases miscounts phrases named `constructor` or `__proto__`, because arbitrary text is stored in a prototype-bearing object. Use a null-prototype dictionary so every phrase is counted as an own key.</violation>
</file>
<file name="client/src/pages/Onboarding.jsx">
<violation number="1" location="client/src/pages/Onboarding.jsx:207">
P2: Reloading or closing while microphone capture is in progress shows no warning, so the in-progress reference recording can be lost. Include recorder activity in this condition by exposing it from `VoiceRecorder` (or emitting a recording-start state).</violation>
</file>
<file name="client/src/components/TextToSpeech.jsx">
<violation number="1" location="client/src/components/TextToSpeech.jsx:139">
P1: A failed speech request now clears the persisted draft and disables the unload safeguard. `handleSpeak` catches generation errors rather than rejecting, so retain the draft unless generation succeeds.</violation>
</file>
<file name="client/src/hooks/useUnsavedChanges.test.js">
<violation number="1" location="client/src/hooks/useUnsavedChanges.test.js:16">
P2: The second test doesn't actually test the hook: it re-implements a copy of handleBeforeUnload inline and asserts on that copy, so a regression in useUnsavedChanges (e.g. a broken effect or wrong listener registration) would still pass. Recommend mounting the hook (e.g. with @testing-library/react) or extracting/importing the real handler so the test verifies the production listener, including that the cleanup removes it when condition toggles false.</violation>
</file>
<file name="docker-compose.yml">
<violation number="1" location="docker-compose.yml:13">
P3: In this production compose service, MOCK_CHATTERBOX=true is a no-op: getIsMock() (server/utils/mock.js) only returns true when NODE_ENV !== 'production', and the README explicitly documents that MOCK_CHATTERBOX has no effect under NODE_ENV=production. As written, the file implies the service will run the offline test stub, but it will actually invoke the live Hugging Face engine. Drop the variable (or set it false) so the production config isn't misleading.</violation>
</file>
<file name="Dockerfile">
<violation number="1" location="Dockerfile:23">
P3: Runtime image retains all client production packages even though it copies only `client/dist`; `npm prune --omit=dev` prunes devDependencies, not dependencies of the client workspace. Reinstall/filter production dependencies for `@voiceforge/server` after the client build so the image does not ship unused browser libraries.</violation>
</file>
<file name="server/index.js">
<violation number="1" location="server/index.js:61">
P2: sendFile error path silently drops the error — request may hang without a response. When `client/dist/index.html` fails to serve (e.g., dist not built, file missing, permissions), the callback calls `next()` without arguments, so Express never reaches the 4-param error handler. The browser receives no response and times out. Pass the error with `next(err)` to let the error handler log it and return a proper 500.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
|
|
||
| await onSpeak(finalText, voice_settings_override); | ||
| setText(""); |
There was a problem hiding this comment.
P1: A failed speech request now clears the persisted draft and disables the unload safeguard. handleSpeak catches generation errors rather than rejecting, so retain the draft unless generation succeeds.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/src/components/TextToSpeech.jsx, line 139:
<comment>A failed speech request now clears the persisted draft and disables the unload safeguard. `handleSpeak` catches generation errors rather than rejecting, so retain the draft unless generation succeeds.</comment>
<file context>
@@ -101,25 +121,26 @@ if (estimatedDuration > 30) {
- setText("");
-}
+ await onSpeak(finalText, voice_settings_override);
+ setText("");
+ try {
+ sessionStorage.removeItem(DRAFT_KEY);
</file context>
| hasServerKey: false, | ||
| }); | ||
|
|
||
| useUnsavedChanges((Boolean(recording?.blob) || isCloning) && !successProfile); |
There was a problem hiding this comment.
P2: Reloading or closing while microphone capture is in progress shows no warning, so the in-progress reference recording can be lost. Include recorder activity in this condition by exposing it from VoiceRecorder (or emitting a recording-start state).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/src/pages/Onboarding.jsx, line 207:
<comment>Reloading or closing while microphone capture is in progress shows no warning, so the in-progress reference recording can be lost. Include recorder activity in this condition by exposing it from `VoiceRecorder` (or emitting a recording-start state).</comment>
<file context>
@@ -203,6 +204,42 @@ export default function Onboarding({ onReady }) {
const isCloning = status === "cloning";
const [serverStatus, setServerStatus] = React.useState({ isMock: false, space: "" });
+ useUnsavedChanges((Boolean(recording?.blob) || isCloning) && !successProfile);
+
+ const handleRecordingReady = React.useCallback((blobArg, metaArg) => {
</file context>
| returnValue: undefined, | ||
| }; | ||
|
|
||
| const handleBeforeUnload = (evt) => { |
There was a problem hiding this comment.
P2: The second test doesn't actually test the hook: it re-implements a copy of handleBeforeUnload inline and asserts on that copy, so a regression in useUnsavedChanges (e.g. a broken effect or wrong listener registration) would still pass. Recommend mounting the hook (e.g. with @testing-library/react) or extracting/importing the real handler so the test verifies the production listener, including that the cleanup removes it when condition toggles false.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/src/hooks/useUnsavedChanges.test.js, line 16:
<comment>The second test doesn't actually test the hook: it re-implements a copy of handleBeforeUnload inline and asserts on that copy, so a regression in useUnsavedChanges (e.g. a broken effect or wrong listener registration) would still pass. Recommend mounting the hook (e.g. with @testing-library/react) or extracting/importing the real handler so the test verifies the production listener, including that the cleanup removes it when condition toggles false.</comment>
<file context>
@@ -0,0 +1,27 @@
+ returnValue: undefined,
+ };
+
+ const handleBeforeUnload = (evt) => {
+ evt.preventDefault();
+ evt.returnValue = "";
</file context>
| if (req.method !== "GET" || req.path.startsWith("/api") || !req.headers.accept?.includes("text/html")) { | ||
| return next(); | ||
| } | ||
| res.sendFile(path.join(staticDistPath, "index.html"), (err) => { |
There was a problem hiding this comment.
P2: sendFile error path silently drops the error — request may hang without a response. When client/dist/index.html fails to serve (e.g., dist not built, file missing, permissions), the callback calls next() without arguments, so Express never reaches the 4-param error handler. The browser receives no response and times out. Pass the error with next(err) to let the error handler log it and return a proper 500.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/index.js, line 61:
<comment>sendFile error path silently drops the error — request may hang without a response. When `client/dist/index.html` fails to serve (e.g., dist not built, file missing, permissions), the callback calls `next()` without arguments, so Express never reaches the 4-param error handler. The browser receives no response and times out. Pass the error with `next(err)` to let the error handler log it and return a proper 500.</comment>
<file context>
@@ -50,6 +50,21 @@ app.get("/api/health", (_request, response) => {
+ if (req.method !== "GET" || req.path.startsWith("/api") || !req.headers.accept?.includes("text/html")) {
+ return next();
+ }
+ res.sendFile(path.join(staticDistPath, "index.html"), (err) => {
+ if (err) {
+ next();
</file context>
| const source = sessionTranscript && sessionTranscript.length > 0 ? sessionTranscript : history; | ||
| const totalSentences = source.length; | ||
| const totalWords = source.reduce((acc, msg) => acc + (msg?.text ? msg.text.split(/\s+/).length : 0), 0); | ||
| const counts = {}; |
There was a problem hiding this comment.
P3: Top Phrases miscounts phrases named constructor or __proto__, because arbitrary text is stored in a prototype-bearing object. Use a null-prototype dictionary so every phrase is counted as an own key.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/src/components/SpeechHistory.jsx, line 55:
<comment>Top Phrases miscounts phrases named `constructor` or `__proto__`, because arbitrary text is stored in a prototype-bearing object. Use a null-prototype dictionary so every phrase is counted as an own key.</comment>
<file context>
@@ -13,15 +14,58 @@ export function SpeechHistory({history,
+ const source = sessionTranscript && sessionTranscript.length > 0 ? sessionTranscript : history;
+ const totalSentences = source.length;
+ const totalWords = source.reduce((acc, msg) => acc + (msg?.text ? msg.text.split(/\s+/).length : 0), 0);
+ const counts = {};
+ source.forEach((msg) => {
+ if (msg?.text) {
</file context>
| const counts = {}; | |
| const counts = Object.create(null); |
| const [tab, setTab] = useState("all"); | ||
| const [search, setSearch] = useState(""); | ||
| const debouncedSearch = useDebounce(search, 300); | ||
| const [selectedTag, setSelectedTag] = useState("All Tags"); |
There was a problem hiding this comment.
P3: A tag literally named All Tags cannot be filtered: selecting it is indistinguishable from the all-tags sentinel. Represent the unfiltered state separately (for example, null) so this valid user tag remains selectable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At client/src/components/SpeechHistory.jsx, line 26:
<comment>A tag literally named `All Tags` cannot be filtered: selecting it is indistinguishable from the all-tags sentinel. Represent the unfiltered state separately (for example, `null`) so this valid user tag remains selectable.</comment>
<file context>
@@ -13,15 +14,58 @@ export function SpeechHistory({history,
const [tab, setTab] = useState("all");
const [search, setSearch] = useState("");
const debouncedSearch = useDebounce(search, 300);
+ const [selectedTag, setSelectedTag] = useState("All Tags");
+ const [analyticsOpen, setAnalyticsOpen] = useState(false);
</file context>
| environment: | ||
| - NODE_ENV=production | ||
| - PORT=3001 | ||
| - MOCK_CHATTERBOX=true |
There was a problem hiding this comment.
P3: In this production compose service, MOCK_CHATTERBOX=true is a no-op: getIsMock() (server/utils/mock.js) only returns true when NODE_ENV !== 'production', and the README explicitly documents that MOCK_CHATTERBOX has no effect under NODE_ENV=production. As written, the file implies the service will run the offline test stub, but it will actually invoke the live Hugging Face engine. Drop the variable (or set it false) so the production config isn't misleading.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docker-compose.yml, line 13:
<comment>In this production compose service, MOCK_CHATTERBOX=true is a no-op: getIsMock() (server/utils/mock.js) only returns true when NODE_ENV !== 'production', and the README explicitly documents that MOCK_CHATTERBOX has no effect under NODE_ENV=production. As written, the file implies the service will run the offline test stub, but it will actually invoke the live Hugging Face engine. Drop the variable (or set it false) so the production config isn't misleading.</comment>
<file context>
@@ -0,0 +1,21 @@
+ environment:
+ - NODE_ENV=production
+ - PORT=3001
+ - MOCK_CHATTERBOX=true
+ - CLIENT_URL=http://localhost:3001
+ restart: unless-stopped
</file context>
| RUN npm run build --workspace client | ||
|
|
||
| # Prune devDependencies to keep runtime node_modules lightweight | ||
| RUN npm prune --omit=dev |
There was a problem hiding this comment.
P3: Runtime image retains all client production packages even though it copies only client/dist; npm prune --omit=dev prunes devDependencies, not dependencies of the client workspace. Reinstall/filter production dependencies for @voiceforge/server after the client build so the image does not ship unused browser libraries.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Dockerfile, line 23:
<comment>Runtime image retains all client production packages even though it copies only `client/dist`; `npm prune --omit=dev` prunes devDependencies, not dependencies of the client workspace. Reinstall/filter production dependencies for `@voiceforge/server` after the client build so the image does not ship unused browser libraries.</comment>
<file context>
@@ -0,0 +1,60 @@
+RUN npm run build --workspace client
+
+# Prune devDependencies to keep runtime node_modules lightweight
+RUN npm prune --omit=dev
+
+
</file context>
❌ Merge Policy ViolationCaution Unauthorized Merge — Pull request #1222 was merged by @Itzzavdheshh (mentor) without any review on record. VoiceForge guidelines require contributors/mentors to submit at least one review (approval, comment, or changes requested) before merging a pull request to ensure code quality and point-tracking integrity. 📊 Violation Summary
🤖 VoiceForge Automation |
🎊 PR Merged SuccessfullyHey @Myparadox-creator! 👋 Congratulations and thank you for your contribution to VoiceForge! Note 🔗 Linked issue(s): #1172 · ✅ Marked as merged and complete Maintainers may still handle final cleanup, release notes, or follow-up tracking after the merge. 🤖 VoiceForge Automation · Updates automatically on edits |
itzzavdhesh
added
gssoc:approved
itzzavdhesh
added
mentor:Anushreebasics
3 participants
🚀 Program
ELUSOC
📝 Description
This PR introduces safeguards to prevent accidental data loss when users refresh or close the browser tab (
F5,Ctrl+W) while composing speech or setting up custom voice clone profiles.Key Changes:
useUnsavedChangesCustom Hook: Created a reusable hook (client/src/hooks/useUnsavedChanges.js) that attaches abeforeunloadwindow listener whenever unsaved changes exist (hasUnsavedChanges === true).TextToSpeech.jsxto sync active composer text intosessionStorage(voiceforge_draft_text), automatically restoring draft text upon page reload.sessionStorageand removesbeforeunloadwarning upon speech submission or manual text clear.useUnsavedChangestoOnboarding.jsxwhile reference voice audio is staged or being processed.useUnsavedChanges.test.jsverifying module exports and event prevention handlers.🔗 Related Issue
Closes #1172
🔄 Type of Change
🧪 How to Test
http://localhost:5173in a browser.TextToSpeech) and type a multi-sentence paragraph.F5) or close the tab (Ctrl+W) -> observe browserbeforeunloadwarning popup ("Changes you made may not be saved").sessionStorage.Enterto speak -> observe draft is cleared fromsessionStorageand subsequent refreshes do not prompt.Onboarding.jsx), record/upload reference audio -> attempt refresh -> observebeforeunloadwarning modal.📸 Screenshots (if applicable)
✅ Checklist
feat: add voice preview)Summary by cubic
Protects in‑progress composer text and voice‑cloning setup from tab close/refresh and restores drafts on reload. Previously, refreshing or closing the tab discarded work; now we show a before‑unload prompt when unsaved input exists and persist drafts in session storage until submission. Drafts previously saved in local storage will not auto‑load. Closes #1172.
useUnsavedChanges; applied inTextToSpeech,VoiceForge, andOnboarding.sessionStorage(voiceforge_draft_text,voiceforge_composer_draft_text) and are cleared on submit.onSpeakonce, removes the Clear button.subtitlesEnabled,subtitleFontSize,subtitleBgOpacity) and wires them to preview.duration/isValid, preserve state during extraction, and remove front‑end size/duration hard limits.useUnsavedChangesmodule.Written for commit c4fb229. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes