Swo 98 backend determine retry for streaming hls video flow #56
Conversation
WalkthroughThis pull request refines error handling, transaction management, and streaming logic across several video processing modules. The updates modify the stream handlers and M3U8 helper functions to use improved error propagation—via enhanced Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant StreamHandler as streamHLSHandler
participant DB as sequelize
participant VideoSvc as VideoService
Client->>StreamHandler: Request video stream
StreamHandler->>DB: Start transaction
StreamHandler->>VideoSvc: finalizeVideo()
VideoSvc-->>StreamHandler: Video finalized
StreamHandler->>VideoSvc: completeTask()
VideoSvc-->>StreamHandler: Task completed
StreamHandler->>DB: Commit transaction
StreamHandler-->>Client: Return success
%% Error scenario:
Note over StreamHandler: On error, rollback transaction and throw CustomError with DATABASE_ERRORS.DB_ERROR
sequenceDiagram
participant Caller
participant FetchUtil as fetchWithError
participant FetchAPI as fetch
Caller->>FetchUtil: call fetchWithError(url, options)
FetchUtil->>FetchAPI: perform fetch(url)
alt Successful Response
FetchAPI-->>FetchUtil: 2xx response
FetchUtil-->>Caller: Return response
else HTTP / Network Error
FetchAPI-->>FetchUtil: Error response / throw error
FetchUtil->>Caller: Throw CustomError with error code and retry flag
end
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
src/services/videos/helpers/m3u8/helpers.ts (1)
219-226: 🛠️ Refactor suggestionUse fetchWithError for consistent error handling.
The function uses direct fetch while other parts of the codebase use fetchWithError. Consider using fetchWithError for consistency:
const streamSegmentFile = async (segmentUrl: string, storagePath: string) => { - const response = await fetch(segmentUrl, { + const response = await fetchWithError(segmentUrl, { timeout: systemConfig.defaultExternalRequestTimeout, size: 32 * 1024 * 1024, // 32MB as safe maximum for high quality segments }); - if (!response.ok || !response.body) { + if (!response.body) { throw new Error(`Failed to fetch segment: ${response.status} ${response.statusText}`); }src/services/videos/helpers/m3u8/helpers.test.ts (1)
590-651: 🛠️ Refactor suggestionUpdate streamSegmentFile tests to use fetchWithError.
For consistency with the implementation, update the tests to use fetchWithError:
test('should throw error when fetch fails', async () => { - const mockResponse = { - ok: false, - body: null, - }; - (fetch as any).mockResolvedValue(mockResponse); + vi.mocked(fetchWithError).mockRejectedValue(new Error('Failed to fetch segment: Network error')); await expect(streamSegmentFile('http://example.com/segment.ts', 'test-path/segment.ts')).rejects.toThrow( - 'Failed to fetch segment' + 'Failed to fetch segment: Network error' ); }); test('should throw error when response body is null', async () => { - const mockResponse = { - ok: true, - body: null, - }; - (fetch as any).mockResolvedValue(mockResponse); + vi.mocked(fetchWithError).mockResolvedValue({ + body: null, + } as any); await expect(streamSegmentFile('http://example.com/segment.ts', 'test-path/segment.ts')).rejects.toThrow( 'Failed to fetch segment' ); });
🧹 Nitpick comments (9)
src/services/videos/helpers/m3u8/index.ts (1)
44-48: Consider adding more fields to thecontext.Having additional identifiers (e.g., user ID, video ID) in the
contextcan help diagnose issues more quickly if broader context is needed for debugging.src/utils/fetch/index.ts (1)
10-55: Implementation provides robust error handling, but consider usingtimeout.The mechanism cleanly distinguishes between server errors, client errors, and network failures. However, the optional
timeoutproperty is not currently implemented. To fully support request timeouts, consider using anAbortControlleror similar approach to enforce the specified timeout.src/services/videos/helpers/m3u8/index.test.ts (3)
24-32: Improve error object creation in CustomError mock.The current mock creates plain Error objects which may cause
instanceofchecks to fail. Consider extending Error properly:- const error = new Error(message); - Object.assign(error, details); - return error; + class CustomErrorMock extends Error { + constructor(message: string, details: any) { + super(message); + Object.assign(this, details); + this.name = 'CustomError'; + } + } + return new CustomErrorMock(message, details);
119-135: Enhance empty segments test coverage.The test verifies error message and error code but doesn't check the duration value. Consider adding an assertion:
expect(CustomError.medium).toHaveBeenCalledWith('Empty HLS content', { errorCode: VIDEO_ERRORS.INVALID_LENGTH, context: expectedContext, source: 'services/videos/helpers/m3u8/index.ts', }); expect(streamPlaylistFile).not.toHaveBeenCalled(); expect(streamSegments).not.toHaveBeenCalled(); + expect(result.duration).toBe(0);
171-189: Enhance error messages in streaming failure tests.The error messages could be more specific to help with debugging. Consider including more context:
- const playlistStreamError = new Error('Playlist stream failed'); + const playlistStreamError = new Error('Failed to stream playlist: network timeout'); vi.mocked(streamPlaylistFile).mockRejectedValue(playlistStreamError); - await expect(streamM3U8(mockM3u8Url, mockStoragePath)).rejects.toThrow('Failed to stream file to storage'); + await expect(streamM3U8(mockM3u8Url, mockStoragePath)).rejects.toThrow('Failed to stream playlist file to storage'); expect(CustomError.medium).toHaveBeenCalledWith('Failed to stream file to storage', { originalError: playlistStreamError, errorCode: VIDEO_ERRORS.STORAGE_UPLOAD_FAILED, shouldRetry: true, context: expectedContext, source: 'services/videos/helpers/m3u8/index.ts', + stack: expect.any(String), // Verify error stack is preserved });src/services/videos/helpers/m3u8/helpers.ts (1)
61-63: Add input validation to parseM3U8Content.The function should validate the m3u8Url parameter to prevent errors with malformed URLs:
const parseM3U8Content = async (m3u8Url: string, excludePatterns: RegExp[] = []): Promise<ParsedResult> => { + if (!m3u8Url || !m3u8Url.match(/^https?:\/\/.+/)) { + throw new Error('Invalid M3U8 URL format'); + } const response = await fetchWithError(m3u8Url); const content = await response.text();src/apps/io/videos/routes/stream-hls/index.test.ts (2)
135-170: Enhance error scenario testing utility.The testErrorScenario function could verify error details more thoroughly:
const testErrorScenario = async ( setupMocks: () => void, expectedErrorCode: string, expectedErrorMessage: string, - checksAfterError?: () => void + checksAfterError?: () => void, + expectedErrorDetails?: Record<string, any> ) => { setupMocks(); let caughtError: Error | null = null; try { await streamHLSHandler(context.mockRequest, context.mockResponse); } catch (error) { caughtError = error as Error; } expect(caughtError).toBeTruthy(); expect(caughtError?.message).toBe(expectedErrorMessage); + expect(caughtError?.stack).toBeTruthy(); if (expectedErrorCode !== VIDEO_ERRORS.CONVERSION_FAILED) { expect(context.mockTransaction.rollback).toHaveBeenCalled(); } // Verify CustomError.critical was called const criticalCalls = vi.mocked(CustomError.critical).mock.calls; expect(criticalCalls.length).toBeGreaterThan(0); // Find the call that matches our error const matchingCall = criticalCalls.find( call => call[0] === expectedErrorMessage && call[1]?.errorCode === expectedErrorCode ); expect(matchingCall).toBeTruthy(); + if (expectedErrorDetails) { + expect(matchingCall?.[1]).toMatchObject(expectedErrorDetails); + } expect(context.mockResponse.json).not.toHaveBeenCalled(); checksAfterError?.(); };
204-267: Add test coverage for edge cases.Consider adding test cases for the following scenarios:
it('should handle transaction commit failure', async () => { await testErrorScenario( () => { const expectedPlayableUrl = 'https://example.com/video.m3u8'; vi.mocked(streamM3U8).mockResolvedValueOnce({ playlistUrl: expectedPlayableUrl, duration: 100, thumbnailUrl: 'cloud-storage-url', }); vi.mocked(finalizeVideo).mockResolvedValueOnce(1); vi.mocked(completeTask).mockResolvedValueOnce(undefined); context.mockTransaction.commit.mockRejectedValueOnce( CustomError.critical('Transaction commit failed', { errorCode: DATABASE_ERRORS.DB_ERROR, shouldRetry: true, }) ); }, DATABASE_ERRORS.DB_ERROR, 'Transaction commit failed' ); }); it('should handle concurrent transaction rollbacks', async () => { await testErrorScenario( () => { context.mockTransaction.rollback.mockRejectedValueOnce(new Error('Already rolled back')); vi.mocked(finalizeVideo).mockRejectedValueOnce( CustomError.critical('Failed to save to database', { errorCode: DATABASE_ERRORS.DB_ERROR, shouldRetry: true, }) ); }, DATABASE_ERRORS.DB_ERROR, 'Failed to save to database' ); });src/services/videos/helpers/m3u8/helpers.test.ts (1)
48-56: Enhance fetch error test coverage.Add test cases for different types of fetch errors:
test('should handle network errors', async () => { vi.mocked(fetchWithError).mockRejectedValue(new Error('Network error')); await expect(parseM3U8Content(baseUrl, excludePatterns)).rejects.toThrow('Network error'); }); test('should handle timeout errors', async () => { vi.mocked(fetchWithError).mockRejectedValue(new Error('Request timeout')); await expect(parseM3U8Content(baseUrl, excludePatterns)).rejects.toThrow('Request timeout'); }); test('should handle invalid content errors', async () => { vi.mocked(fetchWithError).mockResolvedValue({ text: async () => 'Invalid M3U8 content', } as any); await expect(parseM3U8Content(baseUrl, excludePatterns)).rejects.toThrow('Invalid playlist format'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/apps/io/videos/routes/stream-hls/index.test.ts(8 hunks)src/apps/io/videos/routes/stream-hls/index.ts(2 hunks)src/services/videos/helpers/m3u8/helpers.test.ts(12 hunks)src/services/videos/helpers/m3u8/helpers.ts(3 hunks)src/services/videos/helpers/m3u8/index.test.ts(4 hunks)src/services/videos/helpers/m3u8/index.ts(2 hunks)src/utils/custom-error/index.ts(1 hunks)src/utils/error-codes/index.test.ts(1 hunks)src/utils/error-codes/index.ts(2 hunks)src/utils/fetch/index.test.ts(1 hunks)src/utils/fetch/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/services/videos/helpers/m3u8/index.test.ts (1)
Learnt from: shinaBR2
PR: shinaBR2/sworld-backend#54
File: src/services/videos/helpers/thumbnail/index.test.ts:78-85
Timestamp: 2025-02-16T07:46:43.827Z
Learning: In the video processing service tests, error handling verification should focus on core functionality (e.g., operation sequence, error propagation) rather than cleanup operations, letting errors throw naturally.
🔇 Additional comments (22)
src/services/videos/helpers/m3u8/index.ts (4)
50-58: Looks good.Parsation of the M3U8 content and the accompanying logging statements provide helpful insights for debugging and validation.
60-66: No issues here.Throwing an error for empty HLS content is an appropriate approach to indicate a malformed or unexpected playlist.
68-85: Implementation looks correct.Capturing a screenshot from the first included segment and gracefully failing if it cannot be generated aligns with the overall error propagation strategy.
109-115: All set.Using
STORAGE_UPLOAD_FAILEDand marking the error as retryable is consistent with the intended error-handling flow.src/utils/error-codes/index.ts (5)
1-5: Constants defined correctly.The
HTTP_ERRORSobject is well-structured for handling different HTTP-related error categories.
7-9: Database error code introduced properly.Defining
DB_ERRORas a dedicated code is a straightforward way to categorize database-related failures.
16-16: Appropriate addition ofVIDEO_TAKE_SCREENSHOT_FAILED.Including a dedicated error code improves clarity around screenshot-related failures.
33-34: Correct typing strategy.Using mapped types for
HttpErrorCodeandDbErrorCodeleverages the constants while avoiding manual string enumeration.
37-39: Union type export is well-handled.Extending
ErrorCodeto includeHttpErrorCodeandDbErrorCodeunifies error messaging across different domains.src/utils/fetch/index.ts (3)
1-2: Imports look correct.All necessary types and functions for
node-fetchare imported without redundancy.
6-8: Clear type definition.
FetchWrapperOptionsproperly extendsRequestInitwhile allowing a customtimeout?.
57-57: Correct export.Exporting
fetchWithErrorprovides a concise entry point for the improved fetch mechanism.src/apps/io/videos/routes/stream-hls/index.ts (3)
16-16: LGTM! Improved logging placement.Moving the logging statement outside the try-catch block ensures that the event start is always logged, regardless of subsequent errors.
25-26: LGTM! Added transaction management.Good addition of transaction management to ensure database consistency. The transaction is properly committed on success and rolled back on failure.
Also applies to: 38-38, 42-43
44-48:✅ Verification successful
Verify error retry strategy.
The error handling now sets
shouldRetry: truefor database errors. While this is generally a good practice for transient database issues, we should verify if all database errors are truly retryable.
🏁 Script executed:
#!/bin/bash # Search for other database error handling patterns in the codebase rg -A 5 "DATABASE_ERRORS.DB_ERROR" --type tsLength of output: 2098
Database Error Retry Strategy Confirmed
The
shouldRetry: trueflag forDATABASE_ERRORS.DB_ERRORinsrc/apps/io/videos/routes/stream-hls/index.tsis consistent with similar handling in the corresponding test file. The tests explicitly expect a retry flag, which confirms the intended error behavior for transient database issues. Please ensure that only truly transient database errors trigger retries and that permanent errors are managed appropriately.src/utils/error-codes/index.test.ts (3)
5-13: LGTM! Comprehensive HTTP error codes test.Good test coverage for HTTP error codes, ensuring all expected codes are present.
15-23: LGTM! Database error codes test.Good test coverage for database error codes, ensuring the expected code is present.
30-30: LGTM! Updated video error codes.Good addition of
VIDEO_TAKE_SCREENSHOT_FAILEDto the expected video error codes.src/utils/fetch/index.test.ts (3)
7-14: LGTM! Well-structured mock setup.Good mock implementation for node-fetch, providing control over response status and behavior.
46-59: LGTM! Comprehensive error handling tests.Excellent test coverage for different error scenarios:
- Network failures (retryable)
- Server errors (retryable)
- Client errors (non-retryable)
Each test properly verifies error type, code, and retry flag.
Also applies to: 61-77, 79-95
97-113: LGTM! Edge case handling tests.Good coverage of edge cases:
- Preserving existing CustomErrors
- Handling non-Error objects
Tests ensure consistent error wrapping behavior.
Also applies to: 115-127
src/utils/custom-error/index.ts (1)
23-23:✅ Verification successful
Verify impact of broadened error type.
The
originalErrortype has been expanded to includeunknown. While this provides more flexibility, we should verify that all consumers properly handle unknown error types.
🏁 Script executed:
#!/bin/bash # Search for CustomError instantiations with originalError ast-grep --pattern 'new CustomError($_, { $$$, originalError: $_, $$$})'Length of output: 2607
Expanded
originalErrortype – No immediate issues detectedAfter verifying the codebase, all current instantiations of
CustomErrorcontinue to cast or provide a known error type (typically usingerror as Error,error as CustomError, or instances created vianew Error(…)). There are no instances where an error of typeunknownis passed directly. While the updated union type increases flexibility, no consumers are presently affected. Please remain mindful to add type-checks or additional handling if, in the future, errors of truly unknown origin are introduced.
- Location Reviewed:
src/utils/fetch/index.ts(usage witherror as Error)src/utils/error-handler.test.ts(usage withnew Error('Original error'))src/utils/custom-error/index.test.ts(usage with casts to Error or CustomError)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
src/utils/error-codes/index.ts (1)
9-11: Consider expanding database error codes.While
DB_ERRORis a good start, consider adding more specific database error codes likeDB_CONNECTION_ERROR,DB_TRANSACTION_ERROR, etc., to provide more granular error handling for database operations.src/utils/error-codes/index.test.ts (1)
15-23: Consider expanding database error code tests.While the test is correct, it could be expanded when more specific database error codes are added as suggested in the previous review comment.
src/utils/fetch/index.ts (1)
4-53: Add JSDoc documentation for better maintainability.The implementation is robust with good error handling, but would benefit from JSDoc documentation to explain the behavior, parameters, return type, and possible errors.
Add this documentation above the function:
/** * Fetches a resource with enhanced error handling and timeout support. * * @param url - The URL to fetch * @param options - Fetch options with optional timeout * @param options.timeout - Timeout in milliseconds (default: 5000) * @returns Promise resolving to the fetch Response * @throws {CustomError} with appropriate error codes: * - HTTP_ERRORS.SERVER_ERROR for 5xx responses (retryable) * - HTTP_ERRORS.CLIENT_ERROR for 4xx responses (non-retryable) * - HTTP_ERRORS.NETWORK_ERROR for network failures (retryable) */src/utils/fetch/index.test.ts (2)
54-55: Improve test readability by removing redundant assertion.The
expect(true).toBeFalsy()assertion is a confusing way to indicate that the code shouldn't reach this point.Replace with a more descriptive approach:
- expect(true).toBeFalsy(); // Should not reach here + throw new Error('Expected function to throw an error');
138-139: Use a more descriptive error message.The error message could be more descriptive about the expected behavior.
Replace with a more descriptive message:
- expect.fail('Should have thrown an error'); + expect.fail('Expected timeout to trigger a CustomError');src/services/videos/helpers/m3u8/helpers.ts (2)
220-230: Enhance error context for better debugging.The error context could include more details about the failure.
Add response status code to the error context:
throw new CustomError('Failed to fetch segment', { errorCode: HTTP_ERRORS.EMPTY_RESPONSE, shouldRetry: false, - context: { segmentUrl, storagePath, responseStatus: response.statusText }, + context: { + segmentUrl, + storagePath, + responseStatus: response.statusText, + statusCode: response.status + }, });
252-269: Improve error propagation in batch processing.The
streamSegmentsfunction should provide context about which batch failed.Add batch information to help with debugging:
for (let i = 0; i < segmentUrls.length; i += concurrencyLimit) { const batch = segmentUrls.slice(i, i + concurrencyLimit); + const batchNumber = Math.floor(i / concurrencyLimit) + 1; await Promise.all( batch.map(async segmentUrl => { const segmentFileName = segmentUrl.split('/').pop(); const segmentStoragePath = path.join(baseStoragePath, segmentFileName as string); - await streamSegmentFile(segmentUrl, segmentStoragePath); + try { + await streamSegmentFile(segmentUrl, segmentStoragePath); + } catch (error) { + if (error instanceof CustomError) { + error.context = { ...error.context, batchNumber, batchSize: batch.length }; + } + throw error; + } }) ); }src/services/videos/helpers/m3u8/helpers.test.ts (2)
591-595: Use more realistic mock data for better test coverage.The mock response could better represent real-world scenarios.
Use a more realistic mock:
- const mockBody = new ReadableStream(); + const mockBody = new ReadableStream({ + start(controller) { + controller.enqueue(new Uint8Array([0x47, 0x40, 0x00])); // MPEG-TS sync byte and headers + controller.close(); + } + }); (fetchWithError as Mock).mockResolvedValue({ body: mockBody, statusText: 'OK', + status: 200, + headers: new Headers({ + 'content-type': 'video/MP2T', + 'content-length': '3' + }) } as Response);
774-804: Improve test description for clarity.The test description "should log error and throw when segment streaming fails" could be more specific about the failure scenario.
Rename the test for better clarity:
- test('should log error and throw when segment streaming fails', async () => { + test('should stop processing remaining segments when a segment fetch fails with empty response body', async () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
package.json(0 hunks)src/services/videos/helpers/gcp-cloud-storage/index.test.ts(0 hunks)src/services/videos/helpers/m3u8/helpers.test.ts(15 hunks)src/services/videos/helpers/m3u8/helpers.ts(4 hunks)src/utils/error-codes/index.test.ts(1 hunks)src/utils/error-codes/index.ts(2 hunks)src/utils/fetch/index.test.ts(1 hunks)src/utils/fetch/index.ts(1 hunks)
💤 Files with no reviewable changes (2)
- src/services/videos/helpers/gcp-cloud-storage/index.test.ts
- package.json
🔇 Additional comments (9)
src/utils/error-codes/index.ts (4)
1-7: LGTM! Comprehensive HTTP error categorization.The HTTP error codes cover essential network and server-related error scenarios, which will be valuable for the streaming HLS video flow.
18-18: LGTM! Added screenshot error handling.The new
VIDEO_TAKE_SCREENSHOT_FAILEDerror code aligns well with video processing requirements.
35-39: LGTM! Type-safe error code definitions.The type definitions using TypeScript's keyof operator ensure type safety and maintainability.
41-41: LGTM! Comprehensive exports.All necessary types and constants are properly exported.
src/utils/error-codes/index.test.ts (3)
2-2: LGTM! Updated imports.All necessary error code types and constants are properly imported.
5-13: LGTM! Comprehensive HTTP error code testing.The test thoroughly validates all HTTP error codes.
30-30: LGTM! Updated video error code test.The test now includes the new
VIDEO_TAKE_SCREENSHOT_FAILEDerror code.src/utils/fetch/index.ts (1)
4-11:✅ Verification successful
Verify the timeout implementation.
The timeout implementation using
AbortSignal.timeout()is clean, but we should verify browser compatibility.
🌐 Web query:
Is AbortSignal.timeout() supported in all major browsers? What's the minimum browser version required?💡 Result:
The
AbortSignal.timeout()method is supported in modern versions of major browsers but requires specific minimum versions:Browser Compatibility Overview
Browser Minimum Supported Version Notes Chrome 103+ Not supported in Chrome ≤102 Firefox 100+ Not supported in Firefox ≤99 Safari 16.0+ Requires macOS Monterey/iOS 16+ Edge 103+ Not supported in Edge ≤102 Opera 89+ Not supported in Opera ≤88 iOS Safari 16.0+ Requires iPhone/iPad running iOS 16+ Key Details:
Mobile Browsers:
- Chrome for Android and Firefox for Android fully support it[3][4].
- Samsung Internet requires v20+[3][4].
Older Browser Workaround:
UseAbortController+setTimeout()for unsupported environments[2][5]:const controller = new AbortController(); setTimeout(() => controller.abort(), 6000); fetch(url, { signal: controller.signal });
- Timeout Behavior:
Automatically triggers aTimeoutErrorDOMException[1][7], distinct from manual aborts (AbortError)[1][5].Implementation Example:
// Modern approach (supported in listed browsers) try { const response = await fetch(url, { signal: AbortSignal.timeout(5000) // Timeout after 5s }); } catch (err) { if (err.name === 'TimeoutError') { console.error('Request timed out'); } }Browser compatibility data sourced from MDN[1], CanIUse[4], and Chrome release notes[2].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/timeout_static
- 2: https://developer.chrome.com/blog/new-in-chrome-103
- 3: https://www.lambdatest.com/web-technologies/abortcontroller
- 4: https://caniuse.com/mdn-api_abortsignal_timeout_static
- 5: https://betterstack.com/community/guides/scaling-nodejs/nodejs-timeouts/
- 6: https://www.lambdatest.com/abort-controller-&-abort-signal
- 7: https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal
- 8: Timeout AbortSignal (or AbortController) that gets input from the operation whatwg/dom#1082
- 9: https://betterstack.com/community/guides/scaling-nodejs/understanding-abortcontroller/
- 10: https://blog.appsignal.com/2025/02/12/managing-asynchronous-operations-in-nodejs-with-abortcontroller.html
Timeout Implementation & Browser Compatibility Verified
The current implementation leveraging
AbortSignal.timeout()is valid for modern browsers. Our findings confirm that this method is supported in:
- Chrome: v103+
- Firefox: v100+
- Safari / iOS Safari: v16.0+
- Edge: v103+
- Opera: v89+
If you expect users on older browser versions, please consider a fallback (e.g., using
AbortControllerwithsetTimeout()) to ensure broader compatibility.src/services/videos/helpers/m3u8/helpers.ts (1)
125-180:❌ Incorrect review comment
Remove unused code or implement the missing functionality.
The
downloadSegmentsfunction and its helperchunksare marked as potentially unused with TODO comments.Let's verify if these functions are used:
Would you like me to help remove this code or implement the missing functionality?
🏁 Script executed:
#!/bin/bash # Search for usage of downloadSegments function rg -A 5 "downloadSegments\(" --type tsLength of output: 2588
Usage verified; update TODO comments if needed.
The
downloadSegmentsfunction and its helperchunksare actively used—not only in the inline example but also in the tests (seesrc/services/videos/helpers/m3u8/helpers.test.ts). There’s no indication that this code is unused. Please consider revising or removing the TODO comments to avoid confusion about their role.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/utils/fetch/index.ts (1)
16-65: Consider adding request URL to error context for non-CustomError cases.The implementation is solid with comprehensive error handling. However, for consistency, consider adding the request URL to the error context when rethrowing CustomError instances.
if (error instanceof CustomError) { + error.context = { ...error.context, url }; throw error; }src/services/videos/helpers/m3u8/helpers.test.ts (2)
600-668: Consider adding test for network timeout scenario.The tests cover most error scenarios but are missing a test for network timeout, which is a key feature of the new
fetchWithErrorimplementation.Add this test case:
test('should handle network timeout', async () => { const timeoutError = new Error('The operation was aborted'); timeoutError.name = 'AbortError'; (fetchWithError as Mock).mockRejectedValue(timeoutError); await expect(streamSegmentFile(testUrl, testPath)).rejects.toThrow(); expect(CustomError).toHaveBeenCalledWith('Request timed out', { errorCode: HTTP_ERRORS.NETWORK_ERROR, shouldRetry: true, context: { segmentUrl: testUrl, storagePath: testPath, }, }); expect(streamFile).not.toHaveBeenCalled(); });
758-837: Consider testing retry behavior for server errors.The tests verify basic error handling but don't test the retry behavior for server errors (500+).
Add this test case:
test('should handle server errors as retryable', async () => { const segmentUrls = ['http://example.com/seg-1.ts']; const baseStoragePath = 'videos/test'; (fetchWithError as Mock).mockRejectedValueOnce( new CustomError('Server error', { errorCode: HTTP_ERRORS.SERVER_ERROR, shouldRetry: true, context: { url: segmentUrls[0] } }) ); await expect( streamSegments({ segmentUrls, baseStoragePath, options: { concurrencyLimit: 1 }, }) ).rejects.toMatchObject({ shouldRetry: true, errorCode: HTTP_ERRORS.SERVER_ERROR }); expect(streamFile).not.toHaveBeenCalled(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/services/videos/helpers/m3u8/helpers.test.ts(15 hunks)src/services/videos/helpers/m3u8/helpers.ts(4 hunks)src/utils/fetch/index.test.ts(1 hunks)src/utils/fetch/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/utils/fetch/index.test.ts
- src/services/videos/helpers/m3u8/helpers.ts
🔇 Additional comments (2)
src/utils/fetch/index.ts (1)
1-15: LGTM! Well-documented function with clear error handling specifications.The JSDoc documentation clearly outlines the function's behavior, error handling, and return types.
src/services/videos/helpers/m3u8/helpers.test.ts (1)
1-27: LGTM! Well-structured mock implementations.The mock implementations are comprehensive and correctly set up for testing error scenarios.
shinaBR2
deleted the
swo-98-backend-determine-retry-for-streaming-hls-video-flow
branch
Summary by CodeRabbit
New Features
Tests