chore: Apply suggestions from code review

packages/verified-fetch/src/utils/byte-range-context.ts

@@ -48,9 +48,9 @@ export class ByteRangeContext {
   private _isValidRangeRequest: boolean | null = null
   private readonly requestRangeStart: number | null
   private readonly requestRangeEnd: number | null
-  byteStart: number | undefined
-  byteEnd: number | undefined
-  byteSize: number | undefined
+  private byteStart: number | undefined
+  private byteEnd: number | undefined
+  private byteSize: number | undefined
 
   constructor (logger: ComponentLogger, private readonly headers?: HeadersInit) {
     this.log = logger.forComponent('helia:verified-fetch:byte-range-context')
@@ -80,7 +80,7 @@ export class ByteRangeContext {
 
   public setBody (body: SupportedBodyTypes): void {
     this._body = body
-    // if fileSize was set explicitly or already set, don't recalculate it
+    // if fileSize was already set, don't recalculate it
     this.fileSize = this.fileSize ?? getBodySizeSync(body)
 
     this.log.trace('set request body with fileSize %o', this._fileSize)
@@ -111,7 +111,6 @@ export class ByteRangeContext {
       } else if (body instanceof ReadableStream) {
         // stream should already be spliced by dagPb/unixfs
         return body
-        // return splicingTransformStream(body, offset, length, this.logger)
       }
     }
     // offset and length are not set, so not a range request, return body untouched.
@@ -146,7 +145,7 @@ export class ByteRangeContext {
   public set fileSize (size: number | bigint | null) {
     this._fileSize = size != null ? Number(size) : null
     this.log.trace('set _fileSize to %o', this._fileSize)
-    // if fileSize was set explicitly, we need to recalculate the offset details
+    // when fileSize changes, we need to recalculate the offset details
     this.setOffsetDetails()
   }
 
@@ -220,11 +219,10 @@ export class ByteRangeContext {
     if (this.byteStart == null || this.byteStart === 0) {
       return 0
     }
-    // if length is undefined, unixfs.cat and ArrayBuffer.slice will not use an inclusive offset, so we have to subtract by 1
+    // if byteStart is undefined, unixfs.cat and ArrayBuffer.slice will not use an inclusive offset, so we have to subtract by 1
     if (this.isPrefixLengthRequest || this.isSuffixLengthRequest) {
       return this.byteStart - 1
     }
-    // this value will be passed to unixfs.cat
     return this.byteStart
   }
 
@@ -235,8 +233,9 @@ export class ByteRangeContext {
    */
   public get length (): number | undefined {
     // this value will be passed to unixfs.cat.
-    if (this.requestRangeEnd == null) {
-      return undefined // this is a suffix-length request and unixfs has a bug where it doesn't always respect the length parameter
+    if (this.isSuffixLengthRequest) {
+      // this is a suffix-length request and unixfs has a bug where it doesn't always respect the length parameter
+      return undefined
     }
     return this.byteSize ?? undefined
   }

packages/verified-fetch/src/utils/response-headers.ts

@@ -1,17 +1,17 @@
 /**
  * This function returns the value of the `Content-Range` header for a given range.
- * If you know the total size of the body, you should pass it in the `options` object.
+ * If you know the total size of the body, pass it as `byteSize`
  *
  * @see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Range
  */
 export function getContentRangeHeader ({ byteStart, byteEnd, byteSize }: { byteStart: number | undefined, byteEnd: number | undefined, byteSize: number | undefined }): string {
   const total = byteSize ?? '*' // if we don't know the total size, we should use *
 
-  if (byteStart == null && byteEnd == null) {
-    return `bytes */${total}`
-  }
   if (byteStart != null && byteEnd == null) {
-    return `bytes ${byteStart}-*/${total}`
+    if (byteSize == null) {
+      return `bytes */${total}`
+    }
+    return `bytes ${byteStart}-${byteSize}/${byteSize}`
   }
   if (byteStart == null && byteEnd != null) {
     if (byteSize == null) {

packages/verified-fetch/src/verified-fetch.ts

@@ -332,12 +332,13 @@ export class VerifiedFetch {
       }
     }
 
+    // we have to .stat before .cat so we can get the size and make sure byte offsets are calculated properly
     if (byteRangeContext.isRangeRequest && byteRangeContext.isValidRangeRequest) {
       stat = stat ?? await this.unixfs.stat(resolvedCID, {
         signal: options?.signal,
         onProgress: options?.onProgress
       })
-      this.log.trace('stat for %c/%s: %o', resolvedCID, path, stat)
+      this.log.trace('stat.fileSize for rangeRequest %d', stat.fileSize)
       byteRangeContext.fileSize = stat.fileSize
     }
     const offset = byteRangeContext.offset

packages/verified-fetch/test/range-requests.spec.ts

@@ -29,7 +29,6 @@ describe('range requests', () => {
 
   interface SuccessfulTestExpectation {
     contentRange: string
-    // byteSize?: number
     bytes: Uint8Array
   }
   async function testRange (cid: CID, headerRange: string, expected: SuccessfulTestExpectation): Promise<void> {
@@ -49,7 +48,6 @@ describe('range requests', () => {
 
     const responseContent = await response.arrayBuffer()
     expect(new Uint8Array(responseContent)).to.deep.equal(expected.bytes)
-    // expect(responseContent.byteLength).to.equal(expected.byteSize) // the length of the data should match the requested range
   }
 
   async function assertFailingRange (response: Promise<Response>): Promise<void> {