fix: prevent skipped features from being added to chunk features

src/files/shapefile/core/chunkBuilder.ts

@@ -1,5 +1,5 @@
 import { Feature } from 'geojson';
-import { ShapefileChunk } from '../types/index';
+import { FeatureStatus, ShapefileChunk } from '../types/index';
 import { countVertices } from '../../../geo/vertices';
 import { featurePropertiesSchema } from '../../../utils/validation';
 
@@ -18,18 +18,19 @@ export class ChunkBuilder {
     return this.chunkIndex;
   }
 
-  public canAddFeature(feature: Feature): boolean {
+  public canAddFeature(feature: Feature): FeatureStatus {
     this.validateFeatureId(feature);
 
     const featureVertices = countVertices(feature.geometry);
 
     if (featureVertices > this.maxVertices) {
       const featureWithVertices: Feature = { ...feature, properties: { ...feature.properties, vertices: featureVertices } };
       this.skippedFeatures.push(featureWithVertices);
-      return false;
+      return FeatureStatus.SKIPPED;
     }
+    const canAdd = this.currentVerticesCount + featureVertices <= this.maxVertices;
 
-    return this.currentVerticesCount + featureVertices <= this.maxVertices;
+    return canAdd ? FeatureStatus.ADD : FeatureStatus.FULL;
   }
 
   public addFeature(feature: Feature): void {

src/files/shapefile/core/shapeFileReader.ts

@@ -3,7 +3,7 @@
 import { randomUUID } from 'node:crypto';
 import { open } from 'shapefile';
 import { countVertices } from '../../../geo/vertices';
-import { ChunkProcessor, ProcessingState, ProgressInfo, ReaderOptions, ShapefileChunk } from '../types';
+import { FeatureStatus, ChunkProcessor, ProcessingState, ProgressInfo, ReaderOptions, ShapefileChunk } from '../types';
 import { ChunkBuilder } from './chunkBuilder';
 import { IMetricsManager, MetricsManager } from './metricsManager';
 import { IProgressTracker, ProgressTracker } from './progressTracker';
@@ -61,7 +61,16 @@ export class ShapefileChunkReader {
           this.options.logger?.debug({ msg: `Feature ID: ${feature.properties.id}` });
         }
 
-        if (!chunkBuilder.canAddFeature(feature)) {
+        const canAddFeature = chunkBuilder.canAddFeature(feature);
+        if (canAddFeature === FeatureStatus.SKIPPED) {
+          this.options.logger?.warn({
+            msg: `Feature skipped due to exceeding max vertices`,
+            featureId: feature.properties.id,
+          });
+          continue;
+        }
+
+        if (canAddFeature === FeatureStatus.FULL) {
           const readTime = performance.now() - readStart;
           const chunk = chunkBuilder.build();
           this.options.logger?.info({ msg: 'Chunk reading finished', readTime, chunkIndex: chunk.id, featuresCount: chunk.features.length });

src/files/shapefile/types/index.ts

@@ -29,3 +29,13 @@ export interface ReaderOptions {
   /** Metrics collector for performance monitoring */
   metricsCollector?: MetricsCollector;
 }
+
+/* eslint-disable @typescript-eslint/naming-convention */
+export const FeatureStatus = {
+  ADD: 'ADD',
+  FULL: 'FULL',
+  SKIPPED: 'SKIPPED',
+} as const;
+/* eslint-enable @typescript-eslint/naming-convention */
+
+export type FeatureStatus = keyof typeof FeatureStatus;

tests/unit/files/shapefile/chunkBuilder.spec.ts

@@ -1,6 +1,7 @@
 import { Feature, Polygon } from 'geojson';
 import { countVertices } from '../../../../src/geo/vertices';
 import { ChunkBuilder } from '../../../../src/files/shapefile/core/chunkBuilder';
+import { FeatureStatus } from '../../../../src/files/shapefile/types';
 import { createPolygonFeature } from './utils';
 
 describe('ChunkBuilder', () => {
@@ -25,7 +26,7 @@ describe('ChunkBuilder', () => {
   });
 
   describe('canAddFeature', () => {
-    it('should return true when feature can be added within vertex limit', () => {
+    it('should return ADD when feature can be added within vertex limit', () => {
       const feature = createPolygonFeature(
         [
           [0, 0],
@@ -39,7 +40,7 @@ describe('ChunkBuilder', () => {
 
       const canAdd = chunkBuilder.canAddFeature(feature);
 
-      expect(canAdd).toBe(true);
+      expect(canAdd).toBe(FeatureStatus.ADD);
       expect(chunkBuilder.build().skippedFeatures).toHaveLength(0);
     });
 
@@ -55,7 +56,7 @@ describe('ChunkBuilder', () => {
       expect(() => chunkBuilder.canAddFeature(feature)).toThrow('Feature must have an id');
     });
 
-    it('should return false when adding feature would exceed vertex limit', () => {
+    it('should return SKIPPED when adding feature would exceed vertex limit', () => {
       // 11 vertices
       const feature1 = createPolygonFeature(
         [
@@ -76,10 +77,10 @@ describe('ChunkBuilder', () => {
 
       const canAdd = chunkBuilder.canAddFeature(feature1); // Would be 11 > 10
 
-      expect(canAdd).toBe(false);
+      expect(canAdd).toBe(FeatureStatus.SKIPPED);
     });
 
-    it('should return true when adding feature exactly matches vertex limit', () => {
+    it('should return ADD when adding feature exactly matches vertex limit', () => {
       // 10 vertices
       const feature1 = createPolygonFeature(
         [
@@ -99,7 +100,7 @@ describe('ChunkBuilder', () => {
 
       const canAdd = chunkBuilder.canAddFeature(feature1); // Would be exactly 10
 
-      expect(canAdd).toBe(true);
+      expect(canAdd).toBe(FeatureStatus.ADD);
       expect(chunkBuilder.build().skippedFeatures).toHaveLength(0);
     });
 
@@ -124,7 +125,7 @@ describe('ChunkBuilder', () => {
       const canAdd = chunkBuilder.canAddFeature(largeFeature);
       const verticesCount = countVertices(largeFeature.geometry);
 
-      expect(canAdd).toBe(false);
+      expect(canAdd).toBe(FeatureStatus.SKIPPED);
       expect(chunkBuilder.build().skippedFeatures).toStrictEqual([
         { ...largeFeature, properties: { ...largeFeature.properties, vertices: verticesCount } },
       ]);
@@ -150,7 +151,7 @@ describe('ChunkBuilder', () => {
 
       // First, feature gets added to skipped array via canAddFeature
       const canAdd = chunkBuilder.canAddFeature(largeFeature);
-      expect(canAdd).toBe(false);
+      expect(canAdd).toBe(FeatureStatus.SKIPPED);
       expect(chunkBuilder.build().skippedFeatures).toHaveLength(1);
 
       // Now try to add the same feature - it should be ignored
@@ -408,14 +409,14 @@ describe('ChunkBuilder', () => {
       ];
 
       // Add first two features (exactly at limit)
-      expect(chunkBuilder.canAddFeature(features[0])).toBe(true);
+      expect(chunkBuilder.canAddFeature(features[0])).toBe(FeatureStatus.ADD);
       chunkBuilder.addFeature(features[0]);
 
-      expect(chunkBuilder.canAddFeature(features[1])).toBe(true);
+      expect(chunkBuilder.canAddFeature(features[1])).toBe(FeatureStatus.ADD);
       chunkBuilder.addFeature(features[1]);
 
       // Third feature would exceed limit
-      expect(chunkBuilder.canAddFeature(features[2])).toBe(false);
+      expect(chunkBuilder.canAddFeature(features[2])).toBe(FeatureStatus.FULL);
 
       // Build first chunk
       const chunk1 = chunkBuilder.build();
@@ -425,7 +426,7 @@ describe('ChunkBuilder', () => {
 
       // nextChunk and add third feature
       chunkBuilder.nextChunk();
-      expect(chunkBuilder.canAddFeature(features[2])).toBe(true);
+      expect(chunkBuilder.canAddFeature(features[2])).toBe(FeatureStatus.ADD);
       chunkBuilder.addFeature(features[2]);
 
       // Build second chunk
@@ -481,7 +482,7 @@ describe('ChunkBuilder', () => {
         '05fcd4e9-adf5-4258-b582-42ff983b67ce'
       );
 
-      expect(chunkBuilder.canAddFeature(feature)).toBe(true);
+      expect(chunkBuilder.canAddFeature(feature)).toBe(FeatureStatus.ADD);
       chunkBuilder.addFeature(feature);
 
       const chunk = chunkBuilder.build();
@@ -501,7 +502,7 @@ describe('ChunkBuilder', () => {
         '05fcd4e9-adf5-4258-b582-42ff983b67ce'
       );
 
-      expect(chunkBuilder.canAddFeature(feature)).toBe(true);
+      expect(chunkBuilder.canAddFeature(feature)).toBe(FeatureStatus.ADD);
       chunkBuilder.addFeature(feature);
 
       const chunk = chunkBuilder.build();

tests/unit/files/shapefile/shapeFileReader.spec.ts

@@ -8,7 +8,7 @@ import { ShapefileChunkReader } from '../../../../src';
 import { ChunkBuilder } from '../../../../src/files/shapefile/core/chunkBuilder';
 import { MetricsManager } from '../../../../src/files/shapefile/core/metricsManager';
 import { ProgressTracker } from '../../../../src/files/shapefile/core/progressTracker';
-import { ChunkProcessor, ProcessingState, ReaderOptions, ShapefileChunk } from '../../../../src/files/shapefile/types';
+import { ChunkProcessor, FeatureStatus, ProcessingState, ReaderOptions, ShapefileChunk } from '../../../../src/files/shapefile/types';
 import * as vertices from '../../../../src/geo/vertices';
 
 const shapefilePath = '/path/to/shapefile.shp';
@@ -126,7 +126,7 @@ describe('ShapefileChunkReader', () => {
         .mockResolvedValueOnce({ done: false, value: mockFeature })
         .mockResolvedValueOnce({ done: true, value: mockFeature });
 
-      mockChunkBuilder.canAddFeature.mockReturnValue(true);
+      mockChunkBuilder.canAddFeature.mockReturnValue(FeatureStatus.ADD);
       mockChunkBuilder.build.mockReturnValue({
         id: 0,
         features: [mockFeature, mockFeature],
@@ -149,7 +149,10 @@ describe('ShapefileChunkReader', () => {
         .mockResolvedValueOnce({ done: false, value: mockFeature })
         .mockResolvedValueOnce({ done: true, value: mockFeature });
 
-      mockChunkBuilder.canAddFeature.mockReturnValueOnce(true).mockReturnValueOnce(false).mockReturnValueOnce(true);
+      mockChunkBuilder.canAddFeature
+        .mockReturnValueOnce(FeatureStatus.ADD)
+        .mockReturnValueOnce(FeatureStatus.FULL)
+        .mockReturnValueOnce(FeatureStatus.ADD);
 
       const chunk1: ShapefileChunk = { id: 0, features: [mockFeature], skippedFeatures: [], verticesCount: 50 };
       const chunk2: ShapefileChunk = { id: 1, features: [mockFeature, mockFeature], skippedFeatures: [], verticesCount: 100 };
@@ -182,7 +185,7 @@ describe('ShapefileChunkReader', () => {
         .mockResolvedValueOnce({ done: false, value: mockFeature }) // index 4 - process
         .mockResolvedValueOnce({ done: true, value: mockFeature });
 
-      mockChunkBuilder.canAddFeature.mockReturnValue(true);
+      mockChunkBuilder.canAddFeature.mockReturnValue(FeatureStatus.ADD);
       mockChunkBuilder.build.mockReturnValue({
         id: 3,
         features: [mockFeature],
@@ -224,18 +227,19 @@ describe('ShapefileChunkReader', () => {
       };
 
       mockSource.read.mockResolvedValueOnce({ done: false, value: largeFeature }).mockResolvedValueOnce({ done: true, value: largeFeature });
-      mockChunkBuilder.canAddFeature.mockReturnValue(false);
+      mockChunkBuilder.canAddFeature.mockReturnValue(FeatureStatus.SKIPPED);
       mockChunkBuilder.build.mockReturnValue(chunk);
       await reader.readAndProcess(shapefilePath, { process: mockProcessor });
       expect(mockChunkBuilder.canAddFeature).toHaveBeenCalledWith(largeFeature);
       expect(mockChunkBuilder.build).toHaveBeenCalled();
+      expect(mockChunkBuilder.addFeature).not.toHaveBeenCalled();
       expect(mockProcessor).toHaveBeenCalledWith(chunk);
     });
 
     it('should handle processing errors and save state', async () => {
       mockSource.read.mockResolvedValueOnce({ done: false, value: mockFeature }).mockResolvedValueOnce({ done: false, value: mockFeature });
 
-      mockChunkBuilder.canAddFeature.mockReturnValue(false);
+      mockChunkBuilder.canAddFeature.mockReturnValue(FeatureStatus.FULL);
       mockChunkBuilder.build.mockReturnValue({
         id: 0,
         features: [mockFeature],
@@ -260,7 +264,7 @@ describe('ShapefileChunkReader', () => {
     it('should send metrics for each chunk', async () => {
       mockSource.read.mockResolvedValueOnce({ done: false, value: mockFeature }).mockResolvedValueOnce({ done: true, value: mockFeature });
 
-      mockChunkBuilder.canAddFeature.mockReturnValue(true);
+      mockChunkBuilder.canAddFeature.mockReturnValue(FeatureStatus.ADD);
       const chunk = { id: 0, features: [mockFeature], skippedFeatures: [], verticesCount: 50 } as ShapefileChunk;
       mockChunkBuilder.build.mockReturnValue(chunk);
 
@@ -305,7 +309,7 @@ describe('ShapefileChunkReader', () => {
         verticesCount: 0,
       };
       mockSource.read.mockResolvedValueOnce({ done: false, value: largeFeature }).mockResolvedValueOnce({ done: true, value: largeFeature });
-      mockChunkBuilder.canAddFeature.mockReturnValue(false);
+      mockChunkBuilder.canAddFeature.mockReturnValue(FeatureStatus.SKIPPED);
       mockChunkBuilder.build.mockReturnValueOnce(chunk).mockReturnValueOnce(finalChunk);
       mockOptions.generateFeatureId = true;
 
@@ -314,10 +318,65 @@ describe('ShapefileChunkReader', () => {
       expect(mockRandomUUID).toHaveBeenCalledTimes(1);
       expect(mockChunkBuilder.canAddFeature).toHaveBeenCalledWith(largeFeature);
       expect(mockChunkBuilder.build).toHaveBeenCalled();
-      expect(mockChunkBuilder.addFeature).toHaveBeenCalled();
+      expect(mockChunkBuilder.addFeature).not.toHaveBeenCalled();
       expect(mockProcessor).toHaveBeenCalledWith(chunk);
       expect(mockProcessor).toHaveBeenCalledTimes(1);
     });
+
+    it('should not add skipped feature to chunk when it exceeds max vertices and chunk is full', async () => {
+      // This test covers the bug where a skipped feature (exceeding max vertices) could be
+      // incorrectly added to the features array after calling nextChunk() because the
+      // skippedFeatures array was cleared.
+      const normalFeature: Feature = {
+        type: 'Feature',
+        geometry: { type: 'Polygon', coordinates: [[]] },
+        properties: { id: 'normal-feature' },
+      };
+      const largeFeature: Feature = {
+        type: 'Feature',
+        geometry: {
+          type: 'Polygon',
+          coordinates: [
+            [
+              [-1, -1],
+              [-1, 1],
+              [1, 1],
+              [1, -1],
+              [-1, -1],
+              [-1, -1],
+              [-1, -1],
+              [-1, -1],
+            ],
+          ],
+        },
+        properties: { id: 'large-feature' },
+      };
+
+      // Scenario: First feature fills chunk, second feature exceeds max vertices
+      mockSource.read
+        .mockResolvedValueOnce({ done: false, value: normalFeature })
+        .mockResolvedValueOnce({ done: false, value: largeFeature })
+        .mockResolvedValueOnce({ done: true, value: undefined as unknown as Feature });
+
+      // First feature: chunk is full after adding it
+      // Second feature: exceeds max vertices, should be SKIPPED
+      mockChunkBuilder.canAddFeature.mockReturnValueOnce(FeatureStatus.FULL).mockReturnValueOnce(FeatureStatus.SKIPPED);
+
+      const chunk1: ShapefileChunk = { id: 0, features: [normalFeature], skippedFeatures: [], verticesCount: 50 };
+      const chunk2: ShapefileChunk = { id: 1, features: [], skippedFeatures: [largeFeature], verticesCount: 0 };
+      mockChunkBuilder.build.mockReturnValueOnce(chunk1).mockReturnValueOnce(chunk2);
+
+      await reader.readAndProcess(shapefilePath, { process: mockProcessor });
+
+      // Verify the skipped feature was NOT added via addFeature
+      expect(mockChunkBuilder.addFeature).toHaveBeenCalledTimes(1);
+      expect(mockChunkBuilder.addFeature).toHaveBeenCalledWith(normalFeature);
+      expect(mockChunkBuilder.addFeature).not.toHaveBeenCalledWith(largeFeature);
+
+      // Verify both chunks were processed
+      expect(mockProcessor).toHaveBeenCalledTimes(2);
+      expect(mockChunkBuilder.nextChunk).toHaveBeenCalledTimes(1);
+    });
   });
 
   describe('getShapefileStats', () => {