Skip to content

Conversation

@ravish1729
Copy link
Collaborator

@ravish1729 ravish1729 commented Aug 13, 2025

User description

List of Updates:

  • Test cases updated
  • Upload now stream large files without loading them in memory
  • Progress added in upload for NodeJS case

PR Type

Enhancement, Bug fix


Description

  • Stream-based file upload implementation for memory efficiency

  • Progress tracking added for NodeJS uploads

  • Enhanced error handling in access conditions

  • Console logging cleanup in test files


Diagram Walkthrough

flowchart LR
  A["File Upload"] --> B["Stream Processing"]
  B --> C["Progress Tracking"]
  B --> D["Memory Optimization"]
  E["Error Handling"] --> F["Access Conditions"]
  E --> G["Upload Validation"]
Loading

File Walkthrough

Relevant files
Configuration changes
2 files
index.ts
Version bump to 0.4.1                                                                       
+1/-1     
package.json
Version bump to 0.4.1                                                                       
+1/-1     
Error handling
1 files
applyAccessCondition.ts
Enhanced error handling with proper error messages             
+8/-2     
Tests
2 files
encryption.test.ts
Removed console.log statements and fixed error assertions
+1/-5     
upload.test.ts
Updated error message assertion for API failures                 
+1/-1     
Enhancement
3 files
index.ts
Added progress callback parameter to upload function         
+1/-1     
node.ts
Implemented streaming upload with progress tracking           
+58/-52 
util.ts
Added direct stream upload utility function                           
+167/-0 
Formatting
2 files
encryptionBrowser.ts
Removed try-catch blocks and console error logging             
+45/-57 
encryptionNode.ts
Removed try-catch blocks and console error logging             
+59/-71 
Documentation
1 files
README.md
Updated version badge to 0.4.1                                                     
+1/-1     

@codiumai-pr-agent-free
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Error Handling

The error handling try/catch blocks were removed from encryption and decryption functions. This could lead to uncaught exceptions and make debugging more difficult for users when encryption operations fail.

export const encryptFile = async (fileArrayBuffer: any, password: any) => {
  const plainTextBytes = new Uint8Array(fileArrayBuffer)
  const passwordBytes = new TextEncoder().encode(password)

  const salt = window.crypto.getRandomValues(new Uint8Array(16))
  const iv = window.crypto.getRandomValues(new Uint8Array(12))

  const passwordKey = await importKeyFromBytes(passwordBytes)

  const aesKey = await deriveKey(passwordKey, ['encrypt'], {
    name: 'PBKDF2',
    salt: salt,
    iterations: 250000,
    hash: 'SHA-256',
  })
  const cipherBytes = await window.crypto.subtle.encrypt(
    { name: 'AES-GCM', iv: iv },
    aesKey,
    plainTextBytes
  )

  const cipherBytesArray = new Uint8Array(cipherBytes)
  const resultBytes = new Uint8Array(
    cipherBytesArray.byteLength + salt.byteLength + iv.byteLength
  )
  resultBytes.set(salt, 0)
  resultBytes.set(iv, salt.byteLength)
  resultBytes.set(cipherBytesArray, salt.byteLength + iv.byteLength)

  return resultBytes
}
Error Handling

The error handling was modified to check for both !isSuccess and error conditions, but there's no validation that isSuccess exists in the response. If the API response changes, this could lead to unexpected behavior.

if (!isSuccess || error) {
  const errorMessage =
    typeof error === 'string'
      ? error
      : error instanceof Error
      ? error.message
      : JSON.stringify(error)
  throw new Error(errorMessage)
}
Resource Management

The fetchWithDirectStream function creates timeouts and event listeners but doesn't guarantee cleanup in all error scenarios, which could potentially lead to memory leaks.

async function fetchWithDirectStream(
  endpointURL: string,
  options: DirectStreamOptions,
  streamData: {
    boundary: string
    files: Array<{
      stream: any
      filename: string
      size: number
    }>
  }
): Promise<{ data: any }> {
  const {
    method = 'POST',
    headers = {},
    timeout = 7200000,
    onProgress,
  } = options

  const http = eval(`require`)('http')
  const https = eval(`require`)('https')
  const url = eval(`require`)('url')

  const parsedUrl = url.parse(endpointURL)
  const isHttps = parsedUrl.protocol === 'https:'
  const client = isHttps ? https : http

  return new Promise((resolve, reject) => {
    const requestOptions = {
      hostname: parsedUrl.hostname,
      port: parsedUrl.port || (isHttps ? 443 : 80),
      path: parsedUrl.path,
      method,
      headers: {
        ...headers,
        'Content-Type': `multipart/form-data; boundary=${streamData.boundary}`,
      },
    }

    const req = client.request(requestOptions, (res: any) => {
      let data = ''
      res.on('data', (chunk: any) => {
        data += chunk
      })
      res.on('end', () => {
        if (res.statusCode && res.statusCode >= 200 && res.statusCode < 300) {
          try {
            const responseData = JSON.parse(data)
            resolve({ data: responseData })
          } catch (error) {
            reject(new Error('Invalid JSON response'))
          }
        } else {
          reject(new Error(`Request failed with status code ${res.statusCode}`))
        }
      })
    })

    req.on('error', (error: any) => {
      reject(new Error(error.message))
    })

    // Handle timeout
    const timeoutId = setTimeout(() => {
      req.destroy()
      reject(new Error('Request timed out'))
    }, timeout)

    req.on('close', () => {
      clearTimeout(timeoutId)
    })

    // Track total bytes for progress calculation
    let totalBytesUploaded = 0
    let totalBytesToUpload = 0

    // Calculate total size for progress tracking
    if (onProgress) {
      for (const file of streamData.files) {
        totalBytesToUpload += file.size
      }
    }

    // Stream files sequentially with backpressure handling and proper part delimiters
    const writeAsync = (data: string | Buffer): Promise<void> => {
      return new Promise((resolve) => {
        const canWrite = req.write(data)
        if (canWrite) {
          resolve()
        } else {
          req.once('drain', () => resolve())
        }
      })
    }

    const pumpStream = (stream: any): Promise<void> => {
      return new Promise((resolve, rejectPump) => {
        const onData = (chunk: any) => {
          // Update progress if callback is provided
          if (onProgress && totalBytesToUpload > 0) {
            totalBytesUploaded += chunk.length
            const progress = Math.min(
              (totalBytesUploaded / totalBytesToUpload) * 100,
              100
            )
            onProgress({ progress })
          }

          const canWrite = req.write(chunk)
          if (!canWrite) {
            stream.pause()
            req.once('drain', () => stream.resume())
          }
        }
        const onEnd = () => {
          cleanup()
          resolve()
        }
        const onError = (err: any) => {
          cleanup()
          rejectPump(new Error(`File stream error: ${err?.message || err}`))
        }
        const cleanup = () => {
          stream.off('data', onData)
          stream.off('end', onEnd)
          stream.off('error', onError)
        }
        stream.on('data', onData)
        stream.on('end', onEnd)
        stream.on('error', onError)
      })
    }

    ;(async () => {
      try {
        for (let idx = 0; idx < streamData.files.length; idx++) {
          const file = streamData.files[idx]
          const headersPart =
            `--${streamData.boundary}\r\n` +
            `Content-Disposition: form-data; name="file"; filename="${file.filename}"\r\n` +
            `Content-Type: application/octet-stream\r\n\r\n`

          await writeAsync(headersPart)
          await pumpStream(file.stream)
          await writeAsync(`\r\n`)
        }

        await writeAsync(`--${streamData.boundary}--\r\n`)
        req.end()
      } catch (err: any) {
        if (req && !req.destroyed) {
          req.destroy()
        }
        reject(new Error(err?.message || String(err)))
      }
    })()
  })
}

@codiumai-pr-agent-free
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Memory-efficient file uploads

The PR implements stream-based file uploads that prevent large files from being
loaded entirely into memory. This is a significant improvement for handling
large files, as the previous implementation collected all file chunks into
memory buffers before uploading, which could cause memory issues with large
files.

Examples:

src/Lighthouse/upload/files/node.ts [24-106]
export default async (
  sourcePath: string,
  apiKey: string,
  cidVersion: number,
  uploadProgressCallback?: (data: IUploadProgressCallback) => void
): Promise<{ data: IFileUploadedResponse }> => {
  const { createReadStream, lstatSync } = eval(`require`)('fs-extra')
  const path = eval(`require`)('path')

  const token = 'Bearer ' + apiKey

 ... (clipped 64 lines)
src/Lighthouse/utils/util.ts [137-294]
async function fetchWithDirectStream(
  endpointURL: string,
  options: DirectStreamOptions,
  streamData: {
    boundary: string
    files: Array<{
      stream: any
      filename: string
      size: number
    }>

 ... (clipped 36 lines)

Solution Walkthrough:

Before:

async function uploadFile(sourcePath, apiKey) {
  const stream = createReadStream(sourcePath);
  const buffers = [];
  for await (const chunk of stream) {
    buffers.push(chunk);
  }
  const blob = new Blob(buffers);
  const data = new FormData();
  data.append('file', blob);

  await fetchWithTimeout(endpoint, { body: data, ... });
}

After:

async function uploadFile(sourcePath, apiKey) {
  const stream = createReadStream(sourcePath);
  const streamData = {
    files: [{ stream, filename, size }]
  };

  // New function streams the file directly
  await fetchWithDirectStream(endpoint, options, streamData);
}
Suggestion importance[1-10]: 10

__

Why: This suggestion correctly identifies a critical architectural change from in-memory buffering to streaming for file uploads in src/Lighthouse/upload/files/node.ts, which is a fundamental improvement for performance and stability when handling large files.

High
Possible issue
Restore error handling

The decryption function no longer has error handling after the try/catch blocks
were removed. If decryption fails (e.g., with wrong password), the function will
throw an uncaught exception instead of returning undefined as before.

src/Lighthouse/uploadEncrypted/encryptionBrowser.ts [53-78]

 export const decryptFile = async (cipher: any, password: any) => {
-  const cipherBytes = new Uint8Array(cipher)
-  const passwordBytes = new TextEncoder().encode(password)
-
-  const salt = cipherBytes.slice(0, 16)
-  const iv = cipherBytes.slice(16, 16 + 12)
-  const data = cipherBytes.slice(16 + 12)
-  const passwordKey = await importKeyFromBytes(passwordBytes)
-  const aesKey = await deriveKey(passwordKey, ['decrypt'], {
-    name: 'PBKDF2',
-    salt: salt,
-    iterations: 250000,
-    hash: 'SHA-256',
-  })
-
-  const decryptedContent = await window.crypto.subtle.decrypt(
-    {
-      name: 'AES-GCM',
-      iv: iv,
-    },
-    aesKey,
-    data
-  )
-
-  return decryptedContent
+  try {
+    const cipherBytes = new Uint8Array(cipher)
+    const passwordBytes = new TextEncoder().encode(password)
+  
+    const salt = cipherBytes.slice(0, 16)
+    const iv = cipherBytes.slice(16, 16 + 12)
+    const data = cipherBytes.slice(16 + 12)
+    const passwordKey = await importKeyFromBytes(passwordBytes)
+    const aesKey = await deriveKey(passwordKey, ['decrypt'], {
+      name: 'PBKDF2',
+      salt: salt,
+      iterations: 250000,
+      hash: 'SHA-256',
+    })
+  
+    const decryptedContent = await window.crypto.subtle.decrypt(
+      {
+        name: 'AES-GCM',
+        iv: iv,
+      },
+      aesKey,
+      data
+    )
+  
+    return decryptedContent
+  } catch (error) {
+    console.error('Error decrypting file', error)
+    return undefined
+  }
 }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly points out that removing the try...catch block from decryptFile is a significant regression, as it changes the function's behavior from returning undefined on failure to throwing an uncaught exception, which could crash the application.

Medium
Handle undefined error case

The error handling doesn't account for the case when both isSuccess is false and
error is undefined. This could lead to throwing an error with "undefined" as the
message. Add a default error message for this scenario.

src/Lighthouse/encryption/applyAccessCondition.ts [29-37]

 if (!isSuccess || error) {
   const errorMessage =
     typeof error === 'string'
       ? error
       : error instanceof Error
       ? error.message
-      : JSON.stringify(error)
+      : error
+      ? JSON.stringify(error)
+      : "Access control operation failed"
   throw new Error(errorMessage)
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that if !isSuccess is true and error is undefined, an unhelpful error with the message 'undefined' would be thrown, and the proposed change provides a more descriptive default error message.

Low
  • More

@ravish1729 ravish1729 merged commit 8fa5441 into main Aug 13, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants