E2603: Update View Submissions Page#157
Open
CamBhat wants to merge 15 commits intoexpertiza:mainfrom
Open
Conversation
CamBhat
changed the title
Author
|
Moved to draft to work through final project: Intending on
FYI @mcarthur-reece feel free to add more here if needed |
Update ViewSubmissions to fetch from /submitted_content/:id/view_submissions instead of /assignments/:id/view_submissions and update the unit test accordingly. This aligns the frontend with the backend route rename for submitted content.
Use /submitted_content route for submissions
📝 WalkthroughWalkthroughRefactors the assignments area: adds submission and grading routes, implements a folder-based SubmittedContent UI with API integration, replaces placeholder viewers with a real submissions viewer, refactors SubmittedContentService to use axiosClient and FormData handling, updates axios multipart header logic, and adds tests for view submissions and service methods. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client / React
participant API as Backend API
participant Store as Redux Store
Client->>API: loadAssignment(assignmentId)
activate API
API-->>Client: { assignment, dueDate, ... }
deactivate API
Client->>Store: get auth context
Store-->>Client: authenticated user
Client->>API: GET /view_submissions?assignment_id
activate API
API-->>Client: IViewSubmissionsResponse { teams[], ... }
deactivate API
Client->>Client: transformResponse() → ISubmission[]
Client->>Client: Render TanStack table
Note over Client: User clicks team action button
alt Due date passed
Client->>Client: Navigate to /assign-grades?team_id
else
Client->>Client: Navigate to /viewsubmissions?team_id
end
Note over Client: User opens artifact
alt Hyperlink
Client->>Client: Open external URL
else File
Client->>API: downloadFile(fileKey)
activate API
API-->>Client: File blob
deactivate API
Client->>Client: Create object URL & open in new tab
end
sequenceDiagram
participant Client as Client / React
participant API as Backend API
Client->>API: findParticipantContext(assignmentId)
activate API
API-->>Client: { participantId, teamId }
deactivate API
Client->>API: loadFolderContents(currentFolder)
activate API
API-->>Client: { files, folders, current_folder }
deactivate API
Note over Client: User uploads file
Client->>Client: file input onChange
Client->>API: submitFile(participantId, File, currentFolder) [FormData multipart]
activate API
API-->>Client: Success
deactivate API
Client->>API: loadFolderContents(currentFolder)
activate API
API-->>Client: Updated folder contents
deactivate API
Note over Client: User deletes item
Client->>Client: Reconcile summary -> live entry
Client->>API: deleteFile(fileKey)
activate API
API-->>Client: Success
deactivate API
Client->>API: loadFolderContents(currentFolder)
API-->>Client: Updated folder contents
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 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)
Comment |
Prepend the assignment name to the submissions page title. The H1 now renders as "{assignment.name} - View Submissions" instead of just "View Submissions", providing clearer context when reviewing submissions.
Front end updates
Add support for opening and downloading submission artifacts from the submissions table. Import SubmittedContentService and propagate participant_id and folder through transformAsset and transformResponse. Add UI state (activeActionKey, openError) and a handleOpenArtifact function to open external links in a new tab or download files via SubmittedContentService (creating and revoking object URLs). Update artifact rows to include an "Open" button with a loading spinner and stop event propagation on click. Show a dismissible error alert when downloads fail, and include activeActionKey in memo deps to keep button state in sync.
bug fixes recommended by coderabbit review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/pages/Assignments/AssignmentEditor.tsx (1)
226-226: Consider using the typed interface for state.The
calibrationSubmissionsstate usesany[]but should useICalibrationSubmissionRow[]for type safety, since that's whattransformCalibrationSubmissionsreturns.♻️ Proposed type improvement
- const [calibrationSubmissions, setCalibrationSubmissions] = useState<any[]>([]); + const [calibrationSubmissions, setCalibrationSubmissions] = useState<ICalibrationSubmissionRow[]>([]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Assignments/AssignmentEditor.tsx` at line 226, The state declaration uses any[] for calibrationSubmissions which loses type safety; change the useState type to ICalibrationSubmissionRow[] and update the initializer accordingly so calibrationSubmissions and setCalibrationSubmissions use the ICalibrationSubmissionRow type; locate the useState call for calibrationSubmissions and the transformCalibrationSubmissions usage to ensure types align and adjust imports to include ICalibrationSubmissionRow if necessary.src/App.tsx (1)
112-116: Route name and component mismatch may cause confusion.The route path
assignments/:id/viewsubmissionsrendersSubmittedContentinstead ofViewSubmissions. This naming inconsistency could confuse developers maintaining the codebase, as the route name implies it should render theViewSubmissionscomponent (which exists and is imported).Consider either renaming the route or rendering the appropriate component.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.tsx` around lines 112 - 116, The route with path "assignments/:id/viewsubmissions" currently renders SubmittedContent via ProtectedRoute; rename or replace to match intent: either change the element to <ProtectedRoute element={<ViewSubmissions />} /> or rename the path to reflect SubmittedContent. Update the JSX where the route is defined (the object with path "assignments/:id/viewsubmissions", element, and loader: loadAssignment) so the component and route name are consistent (use ViewSubmissions if the route is meant to show submissions).src/pages/Assignments/ViewSubmissions.tsx (1)
277-279: Consider extending the object URL revocation timeout.The 1-second timeout before calling
revokeObjectURLmay be too short for larger files to fully load in the new browser tab, potentially causing the file to fail to render.♻️ Proposed fix to extend timeout
window.setTimeout(() => { window.URL.revokeObjectURL(objectUrl); - }, 1000); + }, 60000); // 60 seconds to allow large files to load🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Assignments/ViewSubmissions.tsx` around lines 277 - 279, The current use of window.setTimeout(() => window.URL.revokeObjectURL(objectUrl), 1000) can revoke the objectUrl too early for large files; change the revocation strategy in ViewSubmissions.tsx by increasing the timeout to a safer value (e.g. 30000 ms) or, better, revoke the URL when the opened window/tab unloads (attach an unload/close listener to the opened window returned by window.open and call window.URL.revokeObjectURL(objectUrl) there) to ensure the blob has finished loading before revocation; update references to window.setTimeout, window.URL.revokeObjectURL, objectUrl and the window.open usage accordingly.src/pages/Assignments/SubmittedContent.tsx (1)
387-389: Consider extending the object URL revocation timeout (same as ViewSubmissions).The 1-second timeout may be too short for large files to fully load in the new browser tab.
♻️ Proposed fix
window.setTimeout(() => { window.URL.revokeObjectURL(objectUrl); - }, 1000); + }, 60000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/Assignments/SubmittedContent.tsx` around lines 387 - 389, The current revocation uses window.setTimeout(() => window.URL.revokeObjectURL(objectUrl), 1000) which can revoke the blob too early for large files; change the timeout to match ViewSubmissions by using the same duration or shared constant (e.g., REVOKE_OBJECT_URL_TIMEOUT) and/or the same helper used there so objectUrl isn't revoked before the new tab finishes loading; update the call around objectUrl and window.URL.revokeObjectURL to use that longer timeout or shared utility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/App.tsx`:
- Around line 328-331: Wrap the route for "assignments/:id/assign-grades" with
the existing ProtectedRoute component and add the same loader used by the other
protected routes so ReviewReportPage requires authentication before rendering;
similarly wrap the "viewsubmissions" and "submitcontent" routes under the /edit/
path with ProtectedRoute and attach the same authenticated loader function used
by the nearby protected routes to ensure consistent protection and data loading.
In `@src/pages/Assignments/SubmittedContent.tsx`:
- Around line 293-297: The effect calling loadFolderContents from useEffect
risks stale closures because loadFolderContents isn't in the dependency array;
wrap loadFolderContents in useCallback (e.g., const loadFolderContents =
useCallback(..., [teamId, preferLiveContents, /* other captured vars */])) so it
updates when those values change, then add loadFolderContents to the useEffect
dependency array (useEffect(..., [currentFolder, participantId,
loadFolderContents])); alternatively, if you prefer not to memoize, change the
effect to call a local function that passes all required values (teamId,
preferLiveContents, etc.) as explicit arguments instead of relying on
closed-over variables.
In `@src/pages/Assignments/ViewSubmissions.tsx`:
- Around line 256-261: SubmittedContentService.downloadFile is being called with
teamId but must use the submission's participant id; replace the second argument
in the download call from teamId to artifact.participantId so
SubmittedContentService.downloadFile(artifact.name, artifact.participantId,
artifact.folder ?? "/") is used (see the artifact produced by transformAsset and
how SubmittedContent.tsx uses participantId) to ensure correct file access for
instructor-view submissions.
In `@src/services/SubmittedContentService.ts`:
- Around line 268-276: formatFileSize currently treats 0 as falsy and returns
"-" for 0-byte files; update the logic in formatFileSize to explicitly handle
null/undefined versus zero by returning "0 Bytes" when bytes === 0 and only
returning "-" when bytes == null (or undefined). Locate the formatFileSize
method and change the initial check to detect null/undefined, add an explicit
bytes === 0 branch that returns "0 Bytes", and keep the existing size
calculation for positive values (ensuring sizeIndex is computed safely from
bytes > 0).
- Around line 103-116: The tests still call
SubmittedContentService.submitFile(formData, '123', '/') but submitFile was
refactored to submitFile(participantId: string, file: File, currentFolder?:
string); update the tests to construct a participantId string and a File (or
mocked File) and call SubmittedContentService.submitFile(participantId, file,
currentFolder) instead of passing a prebuilt FormData. Locate references to
submitFile in test files and replace the single-argument FormData usage with two
separate arguments (participantId and file) so they match the new signature used
in SubmittedContentService.submitFile and its use of
normalizeFolder/currentFolder.
---
Nitpick comments:
In `@src/App.tsx`:
- Around line 112-116: The route with path "assignments/:id/viewsubmissions"
currently renders SubmittedContent via ProtectedRoute; rename or replace to
match intent: either change the element to <ProtectedRoute
element={<ViewSubmissions />} /> or rename the path to reflect SubmittedContent.
Update the JSX where the route is defined (the object with path
"assignments/:id/viewsubmissions", element, and loader: loadAssignment) so the
component and route name are consistent (use ViewSubmissions if the route is
meant to show submissions).
In `@src/pages/Assignments/AssignmentEditor.tsx`:
- Line 226: The state declaration uses any[] for calibrationSubmissions which
loses type safety; change the useState type to ICalibrationSubmissionRow[] and
update the initializer accordingly so calibrationSubmissions and
setCalibrationSubmissions use the ICalibrationSubmissionRow type; locate the
useState call for calibrationSubmissions and the transformCalibrationSubmissions
usage to ensure types align and adjust imports to include
ICalibrationSubmissionRow if necessary.
In `@src/pages/Assignments/SubmittedContent.tsx`:
- Around line 387-389: The current revocation uses window.setTimeout(() =>
window.URL.revokeObjectURL(objectUrl), 1000) which can revoke the blob too early
for large files; change the timeout to match ViewSubmissions by using the same
duration or shared constant (e.g., REVOKE_OBJECT_URL_TIMEOUT) and/or the same
helper used there so objectUrl isn't revoked before the new tab finishes
loading; update the call around objectUrl and window.URL.revokeObjectURL to use
that longer timeout or shared utility.
In `@src/pages/Assignments/ViewSubmissions.tsx`:
- Around line 277-279: The current use of window.setTimeout(() =>
window.URL.revokeObjectURL(objectUrl), 1000) can revoke the objectUrl too early
for large files; change the revocation strategy in ViewSubmissions.tsx by
increasing the timeout to a safer value (e.g. 30000 ms) or, better, revoke the
URL when the opened window/tab unloads (attach an unload/close listener to the
opened window returned by window.open and call
window.URL.revokeObjectURL(objectUrl) there) to ensure the blob has finished
loading before revocation; update references to window.setTimeout,
window.URL.revokeObjectURL, objectUrl and the window.open usage accordingly.
🪄 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: 5b771382-9357-4a6f-9339-4e0df47c2606
📒 Files selected for processing (9)
.gitignoresrc/App.tsxsrc/pages/Assignments/AssignmentEditor.tsxsrc/pages/Assignments/SubmittedContent.csssrc/pages/Assignments/SubmittedContent.tsxsrc/pages/Assignments/ViewSubmissions.tsxsrc/pages/Assignments/__tests__/ViewSubmissions.test.tsxsrc/services/SubmittedContentService.tssrc/utils/axios_client.ts
Comment on lines
+328
to
+331
| { | ||
| path: "assignments/:id/assign-grades", | ||
| element: <ReviewReportPage />, | ||
| }, |
There was a problem hiding this comment.
Missing ProtectedRoute wrapper creates a security gap.
The assign-grades route lacks both ProtectedRoute and a loader. Compare with the new routes at lines 112-121 which properly use ProtectedRoute. This inconsistency allows unauthenticated access to the ReviewReportPage component for grade assignment.
Similarly, the existing routes at lines 103-106 (viewsubmissions) and 108-111 (submitcontent) under the /edit/ path also lack ProtectedRoute, creating an inconsistent security posture.
🔒 Proposed fix to add protection
{
path: "assignments/:id/assign-grades",
- element: <ReviewReportPage />,
+ element: <ProtectedRoute element={<ReviewReportPage />} leastPrivilegeRole={ROLE.TA} />,
+ loader: loadAssignment,
},📝 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
| { | |
| path: "assignments/:id/assign-grades", | |
| element: <ReviewReportPage />, | |
| }, | |
| { | |
| path: "assignments/:id/assign-grades", | |
| element: <ProtectedRoute element={<ReviewReportPage />} leastPrivilegeRole={ROLE.TA} />, | |
| loader: loadAssignment, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/App.tsx` around lines 328 - 331, Wrap the route for
"assignments/:id/assign-grades" with the existing ProtectedRoute component and
add the same loader used by the other protected routes so ReviewReportPage
requires authentication before rendering; similarly wrap the "viewsubmissions"
and "submitcontent" routes under the /edit/ path with ProtectedRoute and attach
the same authenticated loader function used by the nearby protected routes to
ensure consistent protection and data loading.
Comment on lines
+293
to
+297
| useEffect(() => { | ||
| if (!participantId) return; | ||
|
|
||
| loadFolderContents(participantId, currentFolder); | ||
| }, [currentFolder, participantId, teamId]); |
There was a problem hiding this comment.
Missing dependencies in useEffect may cause stale closure issues.
The effect calls loadFolderContents but doesn't include it in the dependency array. This can lead to stale closure bugs where the function captures outdated values of teamId or preferLiveContents.
Consider either:
- Adding
loadFolderContentsto the dependency array (may requireuseCallback) - Passing all required values as arguments explicitly
🐛 Option: Wrap loadFolderContents in useCallback
- const loadFolderContents = async (
+ const loadFolderContents = useCallback(async (
resolvedParticipantId: string,
folderPath: string,
resolvedTeamId: number | null = teamId,
preferLiveDisplay: boolean = preferLiveContents
) => {
// ... existing implementation
- };
+ }, [assignmentId, teamId, preferLiveContents]);Then update the effect:
useEffect(() => {
if (!participantId) return;
loadFolderContents(participantId, currentFolder);
- }, [currentFolder, participantId, teamId]);
+ }, [currentFolder, participantId, teamId, loadFolderContents]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Assignments/SubmittedContent.tsx` around lines 293 - 297, The
effect calling loadFolderContents from useEffect risks stale closures because
loadFolderContents isn't in the dependency array; wrap loadFolderContents in
useCallback (e.g., const loadFolderContents = useCallback(..., [teamId,
preferLiveContents, /* other captured vars */])) so it updates when those values
change, then add loadFolderContents to the useEffect dependency array
(useEffect(..., [currentFolder, participantId, loadFolderContents]));
alternatively, if you prefer not to memoize, change the effect to call a local
function that passes all required values (teamId, preferLiveContents, etc.) as
explicit arguments instead of relying on closed-over variables.
Comment on lines
+256
to
+261
| try { | ||
| const response = await SubmittedContentService.downloadFile( | ||
| artifact.name, | ||
| String(teamId), // use teamId directly as the participant identifier | ||
| artifact.folder ?? "/" | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check SubmittedContentService.downloadFile signature and usage
ast-grep --pattern $'static async downloadFile($_,$_,$_) { $$$ }'
# Also check the backend route pattern expected
rg -n "downloadFile" --type=ts -A 5 src/services/SubmittedContentService.tsRepository: expertiza/reimplementation-front-end
Length of output: 302
🏁 Script executed:
# Check the context around the teamId variable in ViewSubmissions.tsx
rg -n "teamId" src/pages/Assignments/ViewSubmissions.tsx -B 5 -A 2 | head -50
# Search for other usages of downloadFile to see the pattern
rg -n "downloadFile" src/ --type=ts -B 2 -A 2Repository: expertiza/reimplementation-front-end
Length of output: 4898
🏁 Script executed:
# Check the file to understand if it's an instructor view
head -50 src/pages/Assignments/ViewSubmissions.tsxRepository: expertiza/reimplementation-front-end
Length of output: 1381
🏁 Script executed:
# Check the full context of handleOpenArtifact and ISubmissionLink structure
sed -n '240,270p' src/pages/Assignments/ViewSubmissions.tsx
# Check how SubmittedContent.tsx handles the same scenario
sed -n '355,375p' src/pages/Assignments/SubmittedContent.tsx
# Verify that participantId is indeed available in the submission object
rg -n "participantId" src/pages/Assignments/ViewSubmissions.tsx -B 2 -A 2Repository: expertiza/reimplementation-front-end
Length of output: 3056
Replace teamId with artifact.participantId for the downloadFile call.
The SubmittedContentService.downloadFile method expects a participantId parameter, not a team identifier. While teamId is passed to the function, artifact.participantId is available and populated from the submission data (see line 155-158 where participantId is propagated through transformAsset). The SubmittedContent.tsx page correctly uses participantId for the same service call. Using teamId will cause incorrect file access in instructor-view submissions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/Assignments/ViewSubmissions.tsx` around lines 256 - 261,
SubmittedContentService.downloadFile is being called with teamId but must use
the submission's participant id; replace the second argument in the download
call from teamId to artifact.participantId so
SubmittedContentService.downloadFile(artifact.name, artifact.participantId,
artifact.folder ?? "/") is used (see the artifact produced by transformAsset and
how SubmittedContent.tsx uses participantId) to ensure correct file access for
instructor-view submissions.
Comment on lines
268
to
276
| static formatFileSize(bytes: number): string { | ||
| if (bytes === 0) return '0 Bytes'; | ||
| const k = 1024; | ||
| const sizes = ['Bytes', 'KB', 'MB', 'GB']; | ||
| const i = Math.floor(Math.log(bytes) / Math.log(k)); | ||
| return Math.round((bytes / Math.pow(k, i)) * 100) / 100 + ' ' + sizes[i]; | ||
| } | ||
|
|
||
| /** | ||
| * Check if file type is document | ||
| * @param fileName The file name | ||
| * @returns True if file is a document | ||
| */ | ||
| static isDocument(fileName: string): boolean { | ||
| const documentExtensions = ['pdf', 'odt', 'docx', 'md', 'txt']; | ||
| const extension = fileName.split('.').pop()?.toLowerCase(); | ||
| return extension ? documentExtensions.includes(extension) : false; | ||
| } | ||
|
|
||
| /** | ||
| * Check if file type is media | ||
| * @param fileName The file name | ||
| * @returns True if file is media | ||
| */ | ||
| static isMedia(fileName: string): boolean { | ||
| const mediaExtensions = ['png', 'jpeg', 'jpg', 'mp4']; | ||
| const extension = fileName.split('.').pop()?.toLowerCase(); | ||
| return extension ? mediaExtensions.includes(extension) : false; | ||
| } | ||
| if (!bytes) return "-"; | ||
|
|
||
| /** | ||
| * Check if file type is archive | ||
| * @param fileName The file name | ||
| * @returns True if file is an archive | ||
| */ | ||
| static isArchive(fileName: string): boolean { | ||
| const archiveExtensions = ['zip', 'tar', 'gz', '7z']; | ||
| const extension = fileName.split('.').pop()?.toLowerCase(); | ||
| return extension ? archiveExtensions.includes(extension) : false; | ||
| } | ||
| const unit = 1024; | ||
| const sizes = ["Bytes", "KB", "MB", "GB"]; | ||
| const sizeIndex = Math.floor(Math.log(bytes) / Math.log(unit)); | ||
|
|
||
| /** | ||
| * Get icon for file type | ||
| * @param fileName The file name | ||
| * @returns Icon emoji string | ||
| */ | ||
| static getFileIcon(fileName: string): string { | ||
| if (this.isDocument(fileName)) return '📄'; | ||
| if (this.isMedia(fileName)) return '🎬'; | ||
| if (this.isArchive(fileName)) return '📦'; | ||
| return '📁'; | ||
| return `${Math.round((bytes / unit ** sizeIndex) * 100) / 100} ${sizes[sizeIndex]}`; | ||
| } |
There was a problem hiding this comment.
formatFileSize returns "-" for 0-byte files.
The check if (!bytes) on line 269 treats 0 as falsy, so a 0-byte file will display "-" instead of "0 Bytes". If this is intentional, consider adding a comment. Otherwise:
Proposed fix to handle 0 bytes explicitly
static formatFileSize(bytes: number): string {
- if (!bytes) return "-";
+ if (bytes == null || Number.isNaN(bytes)) return "-";
+ if (bytes === 0) return "0 Bytes";
const unit = 1024;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static formatFileSize(bytes: number): string { | |
| if (bytes === 0) return '0 Bytes'; | |
| const k = 1024; | |
| const sizes = ['Bytes', 'KB', 'MB', 'GB']; | |
| const i = Math.floor(Math.log(bytes) / Math.log(k)); | |
| return Math.round((bytes / Math.pow(k, i)) * 100) / 100 + ' ' + sizes[i]; | |
| } | |
| /** | |
| * Check if file type is document | |
| * @param fileName The file name | |
| * @returns True if file is a document | |
| */ | |
| static isDocument(fileName: string): boolean { | |
| const documentExtensions = ['pdf', 'odt', 'docx', 'md', 'txt']; | |
| const extension = fileName.split('.').pop()?.toLowerCase(); | |
| return extension ? documentExtensions.includes(extension) : false; | |
| } | |
| /** | |
| * Check if file type is media | |
| * @param fileName The file name | |
| * @returns True if file is media | |
| */ | |
| static isMedia(fileName: string): boolean { | |
| const mediaExtensions = ['png', 'jpeg', 'jpg', 'mp4']; | |
| const extension = fileName.split('.').pop()?.toLowerCase(); | |
| return extension ? mediaExtensions.includes(extension) : false; | |
| } | |
| if (!bytes) return "-"; | |
| /** | |
| * Check if file type is archive | |
| * @param fileName The file name | |
| * @returns True if file is an archive | |
| */ | |
| static isArchive(fileName: string): boolean { | |
| const archiveExtensions = ['zip', 'tar', 'gz', '7z']; | |
| const extension = fileName.split('.').pop()?.toLowerCase(); | |
| return extension ? archiveExtensions.includes(extension) : false; | |
| } | |
| const unit = 1024; | |
| const sizes = ["Bytes", "KB", "MB", "GB"]; | |
| const sizeIndex = Math.floor(Math.log(bytes) / Math.log(unit)); | |
| /** | |
| * Get icon for file type | |
| * @param fileName The file name | |
| * @returns Icon emoji string | |
| */ | |
| static getFileIcon(fileName: string): string { | |
| if (this.isDocument(fileName)) return '📄'; | |
| if (this.isMedia(fileName)) return '🎬'; | |
| if (this.isArchive(fileName)) return '📦'; | |
| return '📁'; | |
| return `${Math.round((bytes / unit ** sizeIndex) * 100) / 100} ${sizes[sizeIndex]}`; | |
| } | |
| static formatFileSize(bytes: number): string { | |
| if (bytes == null || Number.isNaN(bytes)) return "-"; | |
| if (bytes === 0) return "0 Bytes"; | |
| const unit = 1024; | |
| const sizes = ["Bytes", "KB", "MB", "GB"]; | |
| const sizeIndex = Math.floor(Math.log(bytes) / Math.log(unit)); | |
| return `${Math.round((bytes / unit ** sizeIndex) * 100) / 100} ${sizes[sizeIndex]}`; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/SubmittedContentService.ts` around lines 268 - 276,
formatFileSize currently treats 0 as falsy and returns "-" for 0-byte files;
update the logic in formatFileSize to explicitly handle null/undefined versus
zero by returning "0 Bytes" when bytes === 0 and only returning "-" when bytes
== null (or undefined). Locate the formatFileSize method and change the initial
check to detect null/undefined, add an explicit bytes === 0 branch that returns
"0 Bytes", and keep the existing size calculation for positive values (ensuring
sizeIndex is computed safely from bytes > 0).
Replace raw axios mock with a mocked axios_client instance and update imports. Expand and reorganize SubmittedContentService tests: add detailed submitFile cases (participantId/file args, folder normalization, leading slash handling, empty folder behavior), assert FormData contents, adapt submitHyperlink payload key, change removeHyperlink to use DELETE with data wrapper, verify downloadFile responseType, add deleteFile test section, tighten validateFile/validateUrl assertions, and clarify error-handling tests. Add section comments and clear mocks between tests.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/services/__tests__/SubmittedContentService.test.ts (2)
131-134: TightenlistFilesrequest assertions to prevent false positives.The current
expect.any(Object)is too broad and won’t catch request-parameter regressions.Proposed test hardening
expect(mockedAxios.get).toHaveBeenCalledWith( '/submitted_content/list_files', - expect.any(Object) + expect.objectContaining({ + params: expect.objectContaining({ + id: '123', + current_folder: expect.objectContaining({ name: '/' }), + }), + }) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/SubmittedContentService.test.ts` around lines 131 - 134, The test for listFiles is using expect.any(Object) which is too broad; update the assertion for mockedAxios.get (called with '/submitted_content/list_files') to assert the exact request options or use expect.objectContaining to verify the specific params and headers passed by SubmittedContentService.listFiles (e.g., ensure params include assignmentId/page/pageSize and headers include the expected auth/content-type shape) so request-parameter regressions are caught.
148-153:downloadFiletest should also assert request params.Right now it verifies only
responseType; adding param checks will catch API-shape regressions.Proposed test hardening
expect(mockedAxios.get).toHaveBeenCalledWith( '/submitted_content/download', expect.objectContaining({ + params: expect.objectContaining({ + id: '123', + download: 'test.pdf', + current_folder: expect.objectContaining({ name: '/' }), + }), responseType: 'blob', }) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/SubmittedContentService.test.ts` around lines 148 - 153, Update the downloadFile test to assert that the axios GET request includes the expected request params in addition to responseType: locate the test calling SubmittedContentService.downloadFile and the mockedAxios.get expectation for '/submitted_content/download' and change the matcher to expect.objectContaining that includes responseType: 'blob' plus a params object containing the exact keys/values used in the test call (e.g., content id and filename or whatever variables are passed to downloadFile) so the test verifies both responseType and the request params shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/services/__tests__/SubmittedContentService.test.ts`:
- Around line 131-134: The test for listFiles is using expect.any(Object) which
is too broad; update the assertion for mockedAxios.get (called with
'/submitted_content/list_files') to assert the exact request options or use
expect.objectContaining to verify the specific params and headers passed by
SubmittedContentService.listFiles (e.g., ensure params include
assignmentId/page/pageSize and headers include the expected auth/content-type
shape) so request-parameter regressions are caught.
- Around line 148-153: Update the downloadFile test to assert that the axios GET
request includes the expected request params in addition to responseType: locate
the test calling SubmittedContentService.downloadFile and the mockedAxios.get
expectation for '/submitted_content/download' and change the matcher to
expect.objectContaining that includes responseType: 'blob' plus a params object
containing the exact keys/values used in the test call (e.g., content id and
filename or whatever variables are passed to downloadFile) so the test verifies
both responseType and the request params shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bd1baa19-f5e7-4112-9b62-25e72d8f4300
📒 Files selected for processing (1)
src/services/__tests__/SubmittedContentService.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
E2603: Implement ViewSubmissions frontend
Summary by CodeRabbit
New Features
Improvements
Tests