Conversation
WalkthroughAdds Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue with audio preview functionality by using the internal filename instead of parsing the signed URL to determine the audio file type.
- Updates the
ScribeFileModelinterface to include aninternal_namefield - Simplifies audio type detection by using the
internal_nameproperty
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/types.ts | Adds internal_name field to ScribeFileModel interface |
| src/pages/HistoryDetails.tsx | Uses internal_name instead of parsing signed URL for audio type detection |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/pages/HistoryDetails.tsx(1 hunks)src/types.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
src/types.ts (1)
235-235: LGTM!Adding
internal_nameas a required field is a sound architectural decision. It provides a reliable source for the file extension and avoids the complexity of parsing signed URLs, which may contain query parameters or other URL artifacts.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Deploying care-scribe-fe with
|
| Latest commit: |
c2b2ee5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://589c4f43.care-scribe-fe.pages.dev |
| Branch Preview URL: | https://fix-audio-preview.care-scribe-fe.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/pages/HistoryDetails.tsx (1)
457-464: Fallback MIME derivation is still incorrect for common extensions (e.g.,.mp3→audio/mpeg, notaudio/mp3).
You already preferaudio.mime_type(good), but the extension fallback can still break playback in stricter browsers.<source src={audio.read_signed_url} type={ - audio.mime_type || - "audio/" + (audio.internal_name.split(".").pop() || "mpeg") // Fallback + audio.mime_type || + (() => { + const ext = audio.internal_name.split(".").pop()?.toLowerCase(); + switch (ext) { + case "mp3": + return "audio/mpeg"; + case "wav": + return "audio/wav"; + case "m4a": + case "mp4": + return "audio/mp4"; + case "ogg": + return "audio/ogg"; + default: + return "audio/mpeg"; + } + })() } />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/pages/HistoryDetails.tsx(1 hunks)src/types.ts(1 hunks)src/utils/upload-utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/types.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use strict type checking in TypeScript
Prefer explicit types over inferred types in TypeScript
Files:
src/utils/upload-utils.tssrc/pages/HistoryDetails.tsx
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{ts,tsx}: Define shared types insrc/types.ts
Keep business logic in hooks and pure UI in components
Use absolute imports (configured in vite.config.ts)
Use kebab-case for file names
Include JSDoc comments for complex logic
Follow React hooks rules (dependencies, cleanup, etc.)
Handle cleanup in useEffect hooks
Use React context for state management when needed
Keep state close to where it's used
Files:
src/utils/upload-utils.tssrc/pages/HistoryDetails.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
…into fix-audio-preview
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/utils/upload-utils.ts`:
- Around line 59-67: Derive a non-optional mimeType from blob.type once and
reuse it instead of optional-chaining at call sites: compute const mimeType =
blob.type.split(";")[0] (after the existing check that blob.type is truthy),
keep the existing extension logic (extension =
blob.type.split("/")[1]?.split(";")?.[0] can be adjusted to use mimeType where
appropriate), and then pass mimeType into the CreateFileRequest mime_type field
(replace blob.type?.split(";")?.[0] with mimeType) so the type is strictly
string and no optional chaining remains; update any other places (e.g., around
the name and file creation) to use this single mimeType variable.
- Around line 101-112: The headers object lacks an explicit type and the onError
callback passes an optional/undefined parameter directly to Error; update the
headers declaration to an explicit type (e.g., Record<string, string> or a
suitable Headers type) for the constant named headers, and change the uploadFile
onError callback to accept an optional parameter (e.g., e?: unknown) and call
reject(Error(String(e ?? 'Unknown error'))) or similar so undefined errors
produce a clear message instead of "Error(undefined)"; keep the existing
uploadFile call and onLoad callback intact.
- Around line 29-40: Move the HTTP status checks to run before calling the
success callback: in the xhr.onload handler, first inspect xhr.status and handle
error ranges (400–499 and 500–599) by parsing xhr.responseText with
safeParseJson and invoking onError (using isRecord to iterate messages or a
fallback string) and return early; only call onLoad(xhr) when status is a
successful 2xx code. Ensure you do not call onError after onLoad to avoid
double-callbacks and handle 5xx server errors similarly to 4xx by emitting a
generic server error message if parsing fails.
| xhr.onload = () => { | ||
| onLoad(xhr); | ||
| if (400 <= xhr.status && xhr.status <= 499) { | ||
| const error = JSON.parse(xhr.responseText); | ||
| if (typeof error === "object" && !Array.isArray(error)) { | ||
| const error = safeParseJson(xhr.responseText ?? ""); | ||
| if (isRecord(error)) { | ||
| Object.values(error).forEach((msg) => { | ||
| window.alert(msg || "Something went wrong!"); | ||
| onError(typeof msg === "string" ? msg : "An error occurred."); | ||
| }); | ||
| } else { | ||
| window.alert(error || "Something went wrong!"); | ||
| return; | ||
| } | ||
| onError(typeof error === "string" ? error : "An error occurred"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the upload-utils file
fd -t f upload-utils.tsRepository: ohcnetwork/care_scribe_fe
Length of output: 94
🏁 Script executed:
# View the full file to understand the context
cat -n src/utils/upload-utils.tsRepository: ohcnetwork/care_scribe_fe
Length of output: 4121
🏁 Script executed:
# Search for uploadFile function calls across the codebase
rg -nP --type=ts --type=tsx 'uploadFile\s*\(' -C 3Repository: ohcnetwork/care_scribe_fe
Length of output: 96
🏁 Script executed:
# Search for uploadFile usage across the codebase
rg -n 'uploadFile' --type ts -C 3Repository: ohcnetwork/care_scribe_fe
Length of output: 1056
🏁 Script executed:
# Also search in tsx files (without specifying type to avoid parsing issues)
rg -n 'uploadFile' -C 3 | head -100Repository: ohcnetwork/care_scribe_fe
Length of output: 1056
🏁 Script executed:
# Search for any imports or usage of uploadFile in the entire codebase
rg -n 'from.*upload-utils' -C 2Repository: ohcnetwork/care_scribe_fe
Length of output: 774
🏁 Script executed:
# Also check if uploadFile is used anywhere else beyond uploadScribeFile
rg -n 'uploadFile\(' -C 5Repository: ohcnetwork/care_scribe_fe
Length of output: 681
Move error status checks before onLoad call to prevent double-callback invocation.
The xhr.onload handler currently calls onLoad(xhr) unconditionally (line 30), then checks for 4xx errors and invokes onError afterward. When a 4xx response occurs, both callbacks fire: the caller's onLoad handler (line 110) rejects the Promise on non-2xx status, then onError attempts to reject again. While a Promise only uses the first rejection, this pattern is semantically incorrect and confusing.
The fix should check error status codes first and only invoke onLoad on success (2xx). Additionally, handle 5xx server errors gracefully rather than waiting for network failure.
🔧 Proposed fix
- xhr.onload = () => {
- onLoad(xhr);
- if (400 <= xhr.status && xhr.status <= 499) {
- const error = safeParseJson(xhr.responseText ?? "");
- if (isRecord(error)) {
- Object.values(error).forEach((msg) => {
- onError(typeof msg === "string" ? msg : "An error occurred.");
- });
- return;
- }
- onError(typeof error === "string" ? error : "An error occurred");
- }
- };
+ xhr.onload = () => {
+ if (400 <= xhr.status && xhr.status <= 499) {
+ const error = safeParseJson(xhr.responseText ?? "");
+ if (isRecord(error)) {
+ Object.values(error).forEach((msg) => {
+ onError(typeof msg === "string" ? msg : "An error occurred.");
+ });
+ return;
+ }
+ onError(typeof error === "string" ? error : "An error occurred");
+ return;
+ }
+ if (xhr.status >= 500) {
+ onError(`Upload failed (${xhr.status})`);
+ return;
+ }
+ onLoad(xhr);
+ };🤖 Prompt for AI Agents
In `@src/utils/upload-utils.ts` around lines 29 - 40, Move the HTTP status checks
to run before calling the success callback: in the xhr.onload handler, first
inspect xhr.status and handle error ranges (400–499 and 500–599) by parsing
xhr.responseText with safeParseJson and invoking onError (using isRecord to
iterate messages or a fallback string) and return early; only call onLoad(xhr)
when status is a successful 2xx code. Ensure you do not call onError after
onLoad to avoid double-callbacks and handle 5xx server errors similarly to 4xx
by emitting a generic server error message if parsing fails.
| const extension = blob.type.split("/")[1]?.split(";")?.[0]; | ||
|
|
||
| if (!extension) { | ||
| throw new Error( | ||
| "Could not determine file extension. Got mime type: " + blob.type, | ||
| ); | ||
| } | ||
|
|
||
| const name = `file.${extension}`; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/utils/upload-utils.ts | head -100Repository: ohcnetwork/care_scribe_fe
Length of output: 3488
🏁 Script executed:
rg -nP --type=ts -C3 'createFileUpload'Repository: ohcnetwork/care_scribe_fe
Length of output: 902
🏁 Script executed:
rg -nP --type=ts 'CreateFileRequest' -A10 -B2Repository: ohcnetwork/care_scribe_fe
Length of output: 1820
🏁 Script executed:
rg -nP --type=ts 'mime_type' src/Repository: ohcnetwork/care_scribe_fe
Length of output: 300
Derive mimeType once to keep types strict and avoid optional chaining.
The mime_type field in CreateFileRequest is required (string, non-optional), but the current code at line 88 passes blob.type?.split(";")?.[0] which could be undefined. Since the error check at lines 61–65 guarantees blob.type is truthy before reaching line 88, the refactoring is safe and fixes the type mismatch.
♻️ Suggested refactor
- const extension = blob.type.split("/")[1]?.split(";")?.[0];
+ const mimeType: string = blob.type.split(";")[0];
+ const extension = mimeType.split("/")[1];
@@
- mime_type: blob.type?.split(";")?.[0],
+ mime_type: mimeType,Also applies to: 82–89
🤖 Prompt for AI Agents
In `@src/utils/upload-utils.ts` around lines 59 - 67, Derive a non-optional
mimeType from blob.type once and reuse it instead of optional-chaining at call
sites: compute const mimeType = blob.type.split(";")[0] (after the existing
check that blob.type is truthy), keep the existing extension logic (extension =
blob.type.split("/")[1]?.split(";")?.[0] can be adjusted to use mimeType where
appropriate), and then pass mimeType into the CreateFileRequest mime_type field
(replace blob.type?.split(";")?.[0] with mimeType) so the type is strictly
string and no optional chaining remains; update any other places (e.g., around
the name and file creation) to use this single mimeType variable.
| const headers = { | ||
| "Content-type": newFile?.type?.split(";")?.[0], | ||
| "Content-type": newFile.type.split(";")[0], | ||
| "Content-disposition": "inline", | ||
| }; | ||
|
|
||
| uploadFile( | ||
| url || "", | ||
| url, | ||
| newFile, | ||
| "PUT", | ||
| headers, | ||
| (xhr: XMLHttpRequest) => (xhr.status === 200 ? resolve() : reject()), | ||
| null, | ||
| reject, | ||
| (e) => reject(Error(e)), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and view the specific lines
wc -l src/utils/upload-utils.tsRepository: ohcnetwork/care_scribe_fe
Length of output: 98
🏁 Script executed:
# Read the relevant section of the file
sed -n '90,120p' src/utils/upload-utils.tsRepository: ohcnetwork/care_scribe_fe
Length of output: 860
🏁 Script executed:
# Find the uploadFile function definition to understand the callback signatures
rg -A 10 "function uploadFile|const uploadFile|export.*uploadFile" src/utils/upload-utils.tsRepository: ohcnetwork/care_scribe_fe
Length of output: 715
Add explicit header typing and handle undefined error parameter.
The onError callback receives an optional parameter that may be undefined, resulting in a confusing error message. Also, the headers object lacks explicit typing despite the coding guideline to prefer explicit types over inferred types.
Suggested fix
- const headers = {
+ const headers: Record<string, string> = {
"Content-type": newFile.type.split(";")[0],
"Content-disposition": "inline",
};
@@
- (e) => reject(Error(e)),
+ (e) => reject(new Error(e ?? "Upload failed")),📝 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.
| const headers = { | |
| "Content-type": newFile?.type?.split(";")?.[0], | |
| "Content-type": newFile.type.split(";")[0], | |
| "Content-disposition": "inline", | |
| }; | |
| uploadFile( | |
| url || "", | |
| url, | |
| newFile, | |
| "PUT", | |
| headers, | |
| (xhr: XMLHttpRequest) => (xhr.status === 200 ? resolve() : reject()), | |
| null, | |
| reject, | |
| (e) => reject(Error(e)), | |
| ); | |
| const headers: Record<string, string> = { | |
| "Content-type": newFile.type.split(";")[0], | |
| "Content-disposition": "inline", | |
| }; | |
| uploadFile( | |
| url, | |
| newFile, | |
| headers, | |
| (xhr: XMLHttpRequest) => (xhr.status === 200 ? resolve() : reject()), | |
| (e) => reject(new Error(e ?? "Upload failed")), | |
| ); |
🤖 Prompt for AI Agents
In `@src/utils/upload-utils.ts` around lines 101 - 112, The headers object lacks
an explicit type and the onError callback passes an optional/undefined parameter
directly to Error; update the headers declaration to an explicit type (e.g.,
Record<string, string> or a suitable Headers type) for the constant named
headers, and change the uploadFile onError callback to accept an optional
parameter (e.g., e?: unknown) and call reject(Error(String(e ?? 'Unknown
error'))) or similar so undefined errors produce a clear message instead of
"Error(undefined)"; keep the existing uploadFile call and onLoad callback
intact.
Needs ohcnetwork/care_scribe#25
Summary by CodeRabbit
Refactor
Bug Fixes
Chores