16 subtitle conversion library#17
Conversation
|
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a comprehensive streaming subtitle conversion library that enables real-time subtitle processing and format conversion. The implementation uses Effect-TS for type-safe, streaming-based subtitle processing across multiple output formats.
- Streaming subtitle processing with Effect-TS for type safety and error handling
- Support for multiple subtitle formats (SRT, VTT, JSON, plain text) with incremental output
- Comprehensive filter system for subtitle transformation and processing
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| subtitle-formats.schema.ts | Defines core data structures and schemas for subtitle items, formats, and conversion options |
| subtitle-formats.errors.ts | Error types for subtitle validation, conversion, and processing failures |
| subtitle-filters.ts | Generic filter functions for subtitle transformation and processing pipelines |
| subtitle-converter.ts | Core conversion logic with streaming support and format-specific converters |
| subtitle-converter.test.ts | Comprehensive test suite covering all conversion formats and streaming functionality |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
src/domain/media/subtitle-formats/subtitle-converter.test.ts:1322
- Using an in-memory mock for file system operations in tests is good practice, but ensure real file system integration is tested elsewhere
const memoryFS: Record<string, string> = {};
| * @returns Function that takes a subtitle item, logs it, and returns it unchanged | ||
| */ | ||
| export const debugSubtitle = (label?: string) => (subtitle: SubtitleItem): SubtitleItem => { | ||
| console.log(subtitle) |
There was a problem hiding this comment.
Debug function should include the optional label parameter in the console output for better debugging experience
| console.log(subtitle) | |
| console.log(label ? `[${label}]` : '', subtitle) |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughAdds a complete subtitle processing subsystem: runtime schemas, typed errors, conversion and streaming utilities, composable filters and a simple pipeline, HTTP endpoints and handlers, extensive unit/integration tests, documentation, npm test scripts and a test helper script for JSON/SRT/VTT/plain-text multi-format processing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as HttpApi (/subtitles)
participant Handler as enhancedProcessSubtitlesHandler
participant Converter as SubtitleConverterLive
participant Formatter as Formatters
participant Errors as ErrorMapping
Client->>API: POST /process-enhanced {title, outputFormat, subtitleData, options}
API->>Handler: Validate payload (schema)
Handler->>Handler: parseAndValidateFormats()
Handler->>Handler: validateSubtitleData()
loop per format (parallel)
Handler->>Converter: convert(subtitleData, format, options)
Converter->>Converter: process (validate → transform → merge)
Converter->>Formatter: convertTo(format)
Formatter-->>Converter: content
Converter-->>Handler: content
end
Handler-->>API: MultiFormatResponse
API-->>Client: 200 OK
Handler-->>Errors: catchTags(...)
Errors-->>API: map to 400/422/500
API-->>Client: Error response
sequenceDiagram
autonumber
actor Client
participant API as HttpApi (/subtitles)
participant Handler as processSubtitlesHandler
participant Converter as SubtitleConverterLive
Client->>API: POST /process {title, outputFormat, subtitleData, options}
API->>Handler: Validate payload
Handler->>Converter: convert(subtitleData, format, options)
Converter-->>Handler: content
Handler-->>API: ProcessSubtitlesResponse
API-->>Client: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 29
♻️ Duplicate comments (9)
.vscode/settings.json (1)
2-9: style(vscode): remove opinionated colour overrides from workspace settingsWorkspace-level colour customisations override individual developer preferences and don’t belong in the repo config. Please drop this block.
Apply this diff:
- "workbench.colorCustomizations": { - "editor.selectionBackground": "#135ba2", - "editor.selectionHighlightBackground": "#264f7844", - "editor.findMatchBackground": "#515c6a", - "editor.findMatchHighlightBackground": "#515c6a40", - "editor.findMatchBorder": "#515c6a", - "editor.findMatchHighlightBorder": "#515c6a40" - },src/domain/media/subtitle-formats/subtitle-filters.ts (4)
190-195: Debug function implementation can be improvedThe current implementation matches the past review suggestion but could be more flexible.
56-59: Consider usingOptioninstead of conditional returnThis aligns with the Effect-TS patterns used elsewhere in the codebase. The conditional ternary could be simplified.
-export const filterBySpeaker = - (speakerId: number) => - (subtitle: SubtitleItem): Option.Option<SubtitleItem> => - subtitle.speaker === speakerId ? Option.some(subtitle) : Option.none() +export const filterBySpeaker = + (speakerId: number) => + (subtitle: SubtitleItem): Option.Option<SubtitleItem> => + Option.fromNullable(subtitle.speaker === speakerId ? subtitle : null)
67-73: Consider usingOptionpattern more idiomaticallyThe current implementation could benefit from using Option's built-in methods for cleaner code.
export const filterBySpeakers = (speakerIds: number[]) => (subtitle: SubtitleItem): Option.Option<SubtitleItem> => - typeof subtitle.speaker === 'number' && - speakerIds.includes(subtitle.speaker) - ? Option.some(subtitle) - : Option.none() + Option.fromNullable(subtitle.speaker).pipe( + Option.filter((speaker) => speakerIds.includes(speaker)), + Option.map(() => subtitle) + )
201-216: Consider usingOptioncombinators for validationThe validation logic could be more composable using Option's built-in methods.
export const validateSubtitle = ( subtitle: SubtitleItem, ): Option.Option<SubtitleItem> => { - // Basic validation rules - if (subtitle.start < 0) { - return Option.none() - } - if (subtitle.end <= subtitle.start) { - return Option.none() - } - if (subtitle.text.trim().length === 0) { - return Option.none() - } - return Option.some(subtitle) + return Option.some(subtitle).pipe( + Option.filter((s) => s.start >= 0), + Option.filter((s) => s.end > s.start), + Option.filter((s) => s.text.trim().length > 0) + ) }src/domain/media/subtitle-formats/subtitle-converter.ts (4)
42-48: Consider using Effect Schema's NonEmptyArray validationAs suggested in past reviews, Effect Schema provides utilities for NonEmptyArray validation that would be more idiomatic.
+import { NonEmptyArray } from 'effect/Array' + // Check if subtitles array exists and is not empty - if (!Array.isArray(actualSubtitles) || actualSubtitles.length === 0) { + const nonEmptyResult = NonEmptyArray.fromArray(actualSubtitles) + if (Option.isNone(nonEmptyResult)) { return yield* E.fail( new InvalidSubtitleDataError({ cause: new Error('Subtitle data must be a non-empty array'), }), ) }
99-110: Consider using Schema's NonNegativeInt validationThe manual validation of speaker field could use Effect Schema's NonNegativeInt type for consistency.
The speaker field validation is already defined as
Schema.optional(Schema.NonNegativeInt)in the schema. Consider using Schema validation directly instead of manual checks.
325-326: Inefficient array creation for single item validationCreating a new array with a single item for validation is inefficient, as noted in past reviews.
- Stream.mapEffect((item) => - validateSubtitleData([item], allowEmptyText).pipe(E.map((arr) => arr[0])), - ), + Stream.mapEffect((item) => { + // Validate single item directly without array wrapper + return E.gen(function* () { + if (typeof item.start !== 'number' || typeof item.end !== 'number' || typeof item.text !== 'string') { + return yield* E.fail(new InvalidSubtitleDataError({ + cause: new Error('Invalid subtitle item structure') + })) + } + if (item.start < 0 || item.end < 0) { + return yield* E.fail(new InvalidTimingError({ + cause: new Error('Negative timing values') + })) + } + if (item.start >= item.end) { + return yield* E.fail(new InvalidTimingError({ + cause: new Error('Start time >= end time') + })) + } + if (!allowEmptyText && item.text.trim().length === 0) { + return yield* E.fail(new InvalidSubtitleDataError({ + cause: new Error('Empty text content') + })) + } + return item + }) + }),
657-659: Implement proper schema validation as noted in TODOThe TODO comment indicates incomplete implementation. Schema validation should be properly implemented for production use.
- // For now, skip schema validation to avoid complex Either handling - // In production, you might want to add proper schema validation here + // Validate against schema + const validationResult = yield* E.try({ + try: () => Schema.decodeUnknownSync(SubtitleJson)(subtitles), + catch: (error) => new InvalidSubtitleDataError({ + cause: error instanceof Error ? error : new Error(String(error)) + }) + }) return yield* convertSubtitleFormat(subtitles, format, options)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
.vscode/settings.json(1 hunks)src/domain/media/subtitle-formats/README.md(1 hunks)src/domain/media/subtitle-formats/subtitle-converter.test.ts(1 hunks)src/domain/media/subtitle-formats/subtitle-converter.ts(1 hunks)src/domain/media/subtitle-formats/subtitle-filters.ts(1 hunks)src/domain/media/subtitle-formats/subtitle-formats.errors.ts(1 hunks)src/domain/media/subtitle-formats/subtitle-formats.schema.ts(1 hunks)src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts(1 hunks)src/domain/media/subtitle-formats/subtitle-pipeline-simple.ts(1 hunks)src/domain/media/subtitle-formats/subtitle-streaming.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.ts
📄 CodeRabbit Inference Engine (.rules/code-style.mdc)
src/**/*.ts: Always use relative paths for internal imports; never usesrc/aliases for internal imports in TypeScript files.
Use package imports only for external dependencies in TypeScript files.
Avoid obvious comments; never add comments that just repeat what the code is doing. Only add comments for complex business logic that needs explanation.
Use JSDoc for public APIs, not inline comments for obvious operations.
Add comments only for complex business logic, non-obvious workarounds, edge cases, or important context that affects future changes.
UseEffect.Serviceclass extension pattern for services, include dependencies in service definition, and providemakeTestServicefor testing.
UseE.gen(function* () {})for complex flows; yield effects directly without helper functions; useyield*for effects that return values.
UseData.Taggedfor domain errors, create specific error types for different failure cases, and handle errors explicitly withE.catchTags.
Use Effect's logging utilities instead ofconsole.log; include relevant context in log messages and use appropriate log levels.
Add spans for observability usingE.withSpan, include relevant attributes, and use descriptive span names.
Use PascalCase for service classes (e.g.,BlogStore,SearchStore).
Use camelCase for functions and variables (e.g.,getBlogById,blogPost).
Use UPPERCASE for constants (e.g.,TABLE_NAME,DEFAULT_LIMIT).
Use descriptive names for functions and methods that indicate purpose.
Prefix usecases with action (e.g.,createBlogPostUsecase,getBlogByIdUsecase).
Prefix handlers with HTTP method context (e.g.,_handler,handler).
Use Effect Schema for all data validation; define schemas with explicit types and use branded types for domain identifiers.
Use branded types for domain identifiers and create factory functions with validation.
Access configuration through the Effect system, not directly viaprocess.env.
Use pipe syntax for effect composition;...
Files:
src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.tssrc/domain/media/subtitle-formats/subtitle-streaming.test.tssrc/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-formats.errors.tssrc/domain/media/subtitle-formats/subtitle-formats.schema.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.tssrc/domain/media/subtitle-formats/subtitle-converter.tssrc/domain/media/subtitle-formats/subtitle-filters.ts
src/**/*
📄 CodeRabbit Inference Engine (.rules/code-style.mdc)
Use kebab-case for file names (e.g.,
blog.store.ts,get-blog-by-id.ts).
Files:
src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.tssrc/domain/media/subtitle-formats/README.mdsrc/domain/media/subtitle-formats/subtitle-streaming.test.tssrc/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-formats.errors.tssrc/domain/media/subtitle-formats/subtitle-formats.schema.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.tssrc/domain/media/subtitle-formats/subtitle-converter.tssrc/domain/media/subtitle-formats/subtitle-filters.ts
src/**/*.test.ts
📄 CodeRabbit Inference Engine (.rules/code-style.mdc)
Always use
@effect/vitestfor Effect-based tests; use appropriate test variants (it.effect,it.scoped,it.live); mock services usingmakeTestService.
Files:
src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.tssrc/domain/media/subtitle-formats/subtitle-streaming.test.tssrc/domain/media/subtitle-formats/subtitle-converter.test.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.rules/effect-service-architecture.mdc)
Always use relative paths for internal imports; never use
src/aliases for internal imports. Use package imports only for external dependencies.
Files:
src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.tssrc/domain/media/subtitle-formats/subtitle-streaming.test.tssrc/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-formats.errors.tssrc/domain/media/subtitle-formats/subtitle-formats.schema.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.tssrc/domain/media/subtitle-formats/subtitle-converter.tssrc/domain/media/subtitle-formats/subtitle-filters.ts
**/*.test.ts
📄 CodeRabbit Inference Engine (.rules/effect-testing-patterns.mdc)
**/*.test.ts: Always use@effect/vitestfor tests involving Effect-based code; import test functions (describe,expect,it) from@effect/vitest, not regularvitest.
Always use relative paths for internal imports in test files; never usesrc/aliases.
Use package imports for external dependencies in test files.
Test tagged errors with proper Effect patterns (usingE.exitandgetExitError).
Useit.effectfor tests that need TestContext (TestClock, etc.) in Effect-TS test files.
Useit.scopedfor tests involving resources that need proper cleanup in Effect-TS test files.
Useit.livefor tests that need real system time and live services in Effect-TS test files.
UseFetchHttpClientTestfrom@t6c/lib-apifor mocking HTTP responses in API client tests.
Use mock data from domain packages when available in tests.
Create reusable mock objects for complex data structures in tests.
Use proper branded type constructors for mock data in tests.
Declare variables outside generator functions when used inFetchHttpClientTestto prevent scoping issues in tests.
Use.pipe(E.exit)directly on service calls for cleaner error testing in tests; avoid nestedE.exit(E.gen(...))patterns.
UseExit.isFailure(result)to check for failures andgetExitError(result)to extract error details in tests.
Test all public methods of services, including both success and error scenarios, default parameters, edge cases, and validation.
Use appropriate HTTP status codes (200, 201, 404, 500) and match response structure to actual API responses in mock responses for tests.
Use descriptive test names that explain the scenario, following the pattern: 'should [action] [condition]'.
Group related tests in describe blocks by method.
Reuse mock data across tests when possible.
Create typed mock objects with proper constructors for tests.
Always provide required layers in correct order in tests (e.g.,DefaultWithoutDependenciesfor services,MockConfigLayerfor configurat...
Files:
src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.tssrc/domain/media/subtitle-formats/subtitle-streaming.test.tssrc/domain/media/subtitle-formats/subtitle-converter.test.ts
🧠 Learnings (26)
📚 Learning: 2025-08-10T21:45:57.853Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/code-style.mdc:0-0
Timestamp: 2025-08-10T21:45:57.853Z
Learning: Applies to src/**/*.ts : Use pipe syntax for effect composition; chain operations logically and keep pipes readable with proper formatting.
Applied to files:
src/domain/media/subtitle-formats/README.mdsrc/domain/media/subtitle-formats/subtitle-pipeline-simple.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Test all public methods of services, including both success and error scenarios, default parameters, edge cases, and validation.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:45:57.853Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/code-style.mdc:0-0
Timestamp: 2025-08-10T21:45:57.853Z
Learning: Applies to src/**/*.ts : Write pure functions without side effects; use Effect for operations with side effects; keep business logic separate from infrastructure.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-filters.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Use proper Effect error handling patterns in utility functions.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-converter.tssrc/domain/media/subtitle-formats/subtitle-filters.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Use `it.effect` for tests that need TestContext (TestClock, etc.) in Effect-TS test files.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Use Effect-TS patterns for async operations in utility functions.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-converter.tssrc/domain/media/subtitle-formats/subtitle-filters.ts
📚 Learning: 2025-08-10T21:45:57.853Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/code-style.mdc:0-0
Timestamp: 2025-08-10T21:45:57.853Z
Learning: Applies to src/**/*.ts : Access configuration through the Effect system, not directly via `process.env`.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Always use `effect/vitest` for tests involving Effect-based code; import test functions (`describe`, `expect`, `it`) from `effect/vitest`, not regular `vitest`.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:45:57.853Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/code-style.mdc:0-0
Timestamp: 2025-08-10T21:45:57.853Z
Learning: Applies to src/**/*.ts : Use `E.gen(function* () {})` for complex flows; yield effects directly without helper functions; use `yield*` for effects that return values.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Log errors within the Effect system using `E.logError` or `E.log`, not `console.log`.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:46:54.925Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-store-patterns.mdc:0-0
Timestamp: 2025-08-10T21:46:54.925Z
Learning: Applies to **/*.store.ts : Store classes should integrate with external services (e.g., Algolia, DynamoDB, S3) using Effect-TS patterns
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Use `yield* new ErrorClass()` in Effect.gen instead of throwing errors.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:48:12.331Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/schema-validation.mdc:0-0
Timestamp: 2025-08-10T21:48:12.331Z
Learning: Applies to **/*.types.ts|**/schema/**/*.ts|**/schemas/**/*.ts|**/*-schema.ts|**/*-schemas.ts|**/branded.ts|**/domain/**/*.ts : Use `Data.TaggedError` for domain-specific error types and include relevant identifiers and context in error data
Applied to files:
src/domain/media/subtitle-formats/subtitle-formats.errors.tssrc/domain/media/subtitle-formats/subtitle-formats.schema.ts
📚 Learning: 2025-08-10T21:45:57.853Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/code-style.mdc:0-0
Timestamp: 2025-08-10T21:45:57.853Z
Learning: Applies to src/**/*.ts : Use `Data.Tagged` for domain errors, create specific error types for different failure cases, and handle errors explicitly with `E.catchTags`.
Applied to files:
src/domain/media/subtitle-formats/subtitle-formats.errors.tssrc/domain/media/subtitle-formats/subtitle-formats.schema.ts
📚 Learning: 2025-08-10T21:46:54.925Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-store-patterns.mdc:0-0
Timestamp: 2025-08-10T21:46:54.925Z
Learning: Applies to **/*.store.ts : Convert external errors (e.g., DynamoDB, S3) to domain errors using Effect-TS error handling patterns
Applied to files:
src/domain/media/subtitle-formats/subtitle-formats.errors.ts
📚 Learning: 2025-08-10T21:46:54.925Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-store-patterns.mdc:0-0
Timestamp: 2025-08-10T21:46:54.925Z
Learning: Applies to **/stores/**/*.ts : Convert external errors (e.g., DynamoDB, S3) to domain errors using Effect-TS error handling patterns
Applied to files:
src/domain/media/subtitle-formats/subtitle-formats.errors.ts
📚 Learning: 2025-08-10T21:46:54.925Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-store-patterns.mdc:0-0
Timestamp: 2025-08-10T21:46:54.925Z
Learning: Applies to **/*-store.ts : Convert external errors (e.g., DynamoDB, S3) to domain errors using Effect-TS error handling patterns
Applied to files:
src/domain/media/subtitle-formats/subtitle-formats.errors.ts
📚 Learning: 2025-08-10T21:45:57.853Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/code-style.mdc:0-0
Timestamp: 2025-08-10T21:45:57.853Z
Learning: Applies to src/**/*.ts : Use Effect Schema for all data validation; define schemas with explicit types and use branded types for domain identifiers.
Applied to files:
src/domain/media/subtitle-formats/subtitle-formats.errors.tssrc/domain/media/subtitle-formats/subtitle-formats.schema.tssrc/domain/media/subtitle-formats/subtitle-converter.ts
📚 Learning: 2025-08-10T21:03:54.779Z
Learnt from: BleedingDev
PR: BleedingDev/coursition-effect-experiment#18
File: src/domain/workflow/workflow.errors.ts:3-6
Timestamp: 2025-08-10T21:03:54.779Z
Learning: In Effect-based applications, Schema.TaggedError is used for serializable errors that can bubble up to clients/external systems, while Data.TaggedError is used for internal errors that stay within the application boundaries. This is an intentional architectural pattern to differentiate between public API errors and internal implementation errors.
Applied to files:
src/domain/media/subtitle-formats/subtitle-formats.errors.tssrc/domain/media/subtitle-formats/subtitle-formats.schema.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Never use `throw new Error()` in Effect-TS applications; always create and use `Data.TaggedError` for error handling.
Applied to files:
src/domain/media/subtitle-formats/subtitle-formats.errors.tssrc/domain/media/subtitle-formats/subtitle-formats.schema.ts
📚 Learning: 2025-08-10T21:46:38.101Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-service-architecture.mdc:0-0
Timestamp: 2025-08-10T21:46:38.101Z
Learning: Use tagged errors for type-safe error handling. Convert external errors to domain errors at store boundaries, add context and logging at the usecase level, and map to appropriate HTTP responses at the handler level.
Applied to files:
src/domain/media/subtitle-formats/subtitle-formats.errors.ts
📚 Learning: 2025-08-10T21:46:24.385Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-handler-patterns.mdc:0-0
Timestamp: 2025-08-10T21:46:24.385Z
Learning: Applies to **/lambdas/**/handler.ts,**/*-handler.ts : Handlers must map domain errors to HTTP responses using 'E.catchTags' with appropriate error-to-response mappings.
Applied to files:
src/domain/media/subtitle-formats/subtitle-formats.errors.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Test tagged errors with proper Effect patterns (using `E.exit` and `getExitError`).
Applied to files:
src/domain/media/subtitle-formats/subtitle-formats.errors.ts
📚 Learning: 2025-08-10T21:48:12.331Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/schema-validation.mdc:0-0
Timestamp: 2025-08-10T21:48:12.331Z
Learning: Applies to **/*.types.ts|**/schema/**/*.ts|**/schemas/**/*.ts|**/*-schema.ts|**/*-schemas.ts|**/branded.ts|**/domain/**/*.ts : Always use `Schema as S` for Effect Schema imports
Applied to files:
src/domain/media/subtitle-formats/subtitle-formats.schema.tssrc/domain/media/subtitle-formats/subtitle-converter.ts
📚 Learning: 2025-08-10T21:46:10.977Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/cursor-rule-maintainer.mdc:0-0
Timestamp: 2025-08-10T21:46:10.977Z
Learning: Applies to .cursor/rules/schema-validation.mdc : Add new Effect Schema patterns, validation improvements, branded type patterns, and error handling for schemas to `schema-validation.mdc`
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.ts
📚 Learning: 2025-08-10T21:48:12.331Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/schema-validation.mdc:0-0
Timestamp: 2025-08-10T21:48:12.331Z
Learning: Applies to **/schema/**/*.ts|**/schemas/**/*.ts|**/*-schema.ts|**/*-schemas.ts : Test schema validation with valid and invalid inputs using effect/vitest and Effect-based test patterns
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.ts
🧬 Code Graph Analysis (9)
src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts (3)
src/domain/media/subtitle-formats/subtitle-formats.schema.ts (2)
SubtitleItem(7-16)SubtitleItem(64-64)src/domain/media/subtitle-formats/subtitle-pipeline-simple.ts (13)
createPipeline(266-268)SubtitlePipeline(144-257)createArrayPipeline(273-279)PipelineConfig(43-47)createCollector(121-128)applyFilters(79-93)formatToSrt(288-306)formatToVtt(311-324)formatToPlainText(329-331)formatToJson(336-338)processToSrt(373-382)processToVtt(387-396)processWithConfig(401-415)src/domain/media/subtitle-formats/subtitle-filters.ts (9)
toUpperCase(149-152)applyFilters(348-373)removeEmptySubtitles(179-182)addPrefix(81-86)filterBySpeakers(67-73)addTimingOffset(42-48)transformText(137-142)capitalize(169-172)validateSubtitle(202-216)
src/domain/media/subtitle-formats/README.md (1)
src/stores/media/media.store.test.ts (2)
describe(17-49)Effect(7-14)
src/domain/media/subtitle-formats/subtitle-streaming.test.ts (3)
src/domain/media/subtitle-formats/subtitle-formats.schema.ts (2)
SubtitleItem(7-16)SubtitleItem(64-64)src/domain/media/subtitle-formats/subtitle-filters.ts (14)
collectStream(395-401)createSubtitleStream(315-316)applyFilters(348-373)toUpperCase(149-152)removeEmptySubtitles(179-182)addTimingOffset(42-48)processAndCollect(410-415)filterBySpeaker(56-59)processSubtitlesParallel(425-430)addPrefix(81-86)streamSubtitlesGenerator(439-467)validateSubtitle(202-216)collectGenerator(475-483)processSubtitlesWithGenerator(492-500)src/domain/media/subtitle-formats/subtitle-pipeline-simple.ts (1)
applyFilters(79-93)
src/domain/media/subtitle-formats/subtitle-converter.test.ts (5)
src/domain/media/subtitle-formats/subtitle-formats.schema.ts (2)
SubtitleItem(7-16)SubtitleItem(64-64)src/domain/media/subtitle-formats/subtitle-converter.ts (7)
SubtitleItem(712-712)validateSubtitleData(25-122)processSubtitles(252-309)SubtitleConverterLive(631-709)runSubtitleProcessingStream(351-359)runSubtitleConversionStream(446-453)addSpeakerInfo(159-170)src/domain/media/subtitle-formats/subtitle-filters.ts (6)
applyFiltersToArray(225-264)replaceText(16-34)addTimingOffset(42-48)filterBySpeaker(56-59)addPrefix(81-86)streamSubtitles(273-302)src/domain/media/subtitle-formats/subtitle-formats.errors.ts (2)
UnsupportedFormatError(43-48)InvalidTimingError(50-52)src/domain/media/subtitle-formats/subtitle-pipeline-simple.ts (1)
filter(169-172)
src/domain/media/subtitle-formats/subtitle-formats.errors.ts (2)
src/domain/media/media.errors.ts (6)
MediaParsingError(23-26)MediaClientError(28-31)MediaEmpty(4-7)YtDlpValidationError(37-41)MediaEmptyError(15-17)YtDlpDownloadError(33-35)src/domain/workflow/workflow.errors.ts (1)
WorkflowError(3-6)
src/domain/media/subtitle-formats/subtitle-formats.schema.ts (4)
src/domain/media/subtitle-formats/subtitle-converter.ts (2)
SubtitleItem(712-712)SubtitleJson(712-712)src/steps/media/persist-subtitles.step.ts (1)
subtitlesJson(3-14)src/stores/media/media.store.test.ts (4)
describe(17-49)it(18-48)Effect(19-31)Effect(7-14)src/domain/media/media.errors.ts (3)
MediaEmpty(4-7)MediaParsingError(23-26)MediaClientError(28-31)
src/domain/media/subtitle-formats/subtitle-pipeline-simple.ts (3)
src/domain/media/subtitle-formats/subtitle-formats.schema.ts (2)
SubtitleItem(7-16)SubtitleItem(64-64)src/domain/media/subtitle-formats/subtitle-filters.ts (1)
applyFilters(348-373)src/steps/media/persist-subtitles.step.ts (1)
subtitlesJson(3-14)
src/domain/media/subtitle-formats/subtitle-converter.ts (2)
src/domain/media/subtitle-formats/subtitle-formats.schema.ts (11)
SubtitleJson(21-21)SubtitleJson(65-65)SubtitleItem(7-16)SubtitleItem(64-64)ConversionOptions(31-42)ConversionOptions(67-67)SubtitleFormat(26-26)SubtitleFormat(66-66)SubtitleConversionResultSchema(54-57)MultipleFormatResult(59-61)MultipleFormatResult(68-70)src/domain/media/subtitle-formats/subtitle-formats.errors.ts (4)
InvalidSubtitleDataError(37-41)InvalidTimingError(50-52)UnsupportedFormatError(43-48)ConversionError(54-57)
src/domain/media/subtitle-formats/subtitle-filters.ts (5)
src/domain/media/subtitle-formats/subtitle-formats.schema.ts (2)
SubtitleItem(7-16)SubtitleItem(64-64)src/domain/media/subtitle-formats/subtitle-converter.ts (3)
SubtitleItem(712-712)formatTimeSrt(461-467)formatTimeVtt(475-481)src/domain/media/subtitle-formats/subtitle-pipeline-simple.ts (3)
filter(169-172)stream(161-164)applyFilters(79-93)src/steps/media/persist-subtitles.step.ts (1)
subtitlesJson(3-14)src/usecases/media/transcribe-workflow.usecase.ts (2)
transcribeMediaStep(51-52)persistSubtitlesStep(55-56)
🪛 LanguageTool
src/domain/media/subtitle-formats/README.md
[style] ~80-~80: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...peline. #### .parallelFilter(filter) Adds a parallel filter stage to the pipeline...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~83-~83: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...pipeline. #### .collector(collector) Adds a collector stage to the pipeline. ###...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~86-~86: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...pipeline. #### .formatter(formatter) Adds a formatter stage to the pipeline. ###...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~100-~100: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ssWithConfig(items, filters?, config?)` Processes subtitles with custom configuration. #...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~246-~246: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...e buffer sizes - Consider streaming for very large datasets - Clean up references after pr...
(EN_WEAK_ADJECTIVE)
[grammar] ~278-~278: The verb ‘Ensure’ is plural. Did you mean: “ensures”? Did you use a verb instead of a noun?
Context: ...e tests 3. Update this documentation 4. Ensure type safety 5. Consider performance imp...
(PLURAL_VERB_AFTER_THIS)
🪛 markdownlint-cli2 (0.17.2)
src/domain/media/subtitle-formats/README.md
66-66: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
69-69: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
74-74: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
77-77: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
80-80: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
83-83: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
86-86: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
89-89: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
94-94: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
97-97: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
100-100: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
105-105: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
106-106: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
114-114: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
115-115: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
119-119: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
120-120: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
123-123: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
124-124: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
127-127: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
128-128: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
132-132: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
133-133: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
133-133: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
143-143: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
144-144: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
144-144: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
154-154: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
155-155: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
166-166: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
167-167: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
167-167: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
174-174: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
175-175: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
183-183: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
184-184: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
199-199: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
200-200: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
218-218: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
219-219: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
239-239: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
240-240: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
244-244: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
245-245: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
249-249: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
250-250: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
279-279: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
279-279: Files should end with a single newline character
(MD047, single-trailing-newline)
🔇 Additional comments (3)
src/domain/media/subtitle-formats/subtitle-formats.errors.ts (1)
3-35: feat(errors): good split between API errors (Schema.TaggedError) and internal domain errors (Data.TaggedError)This follows our error architecture and is consistent with media/workflow error patterns.
src/domain/media/subtitle-formats/subtitle-formats.schema.ts (1)
54-61: refactor(schema): keep schema constants and DTO schemas clearly namedMinor: you already have SubtitleConversionResult as a Data class and a corresponding schema. Consider naming the schema explicitly for clarity.
-export const SubtitleConversionResultSchema = Schema.Struct({ - format: SubtitleFormat, - content: Schema.String, -}) +export const SubtitleConversionResultSchema = S.Struct({ + format: SubtitleFormat, + content: S.String, +})⛔ Skipped due to learnings
Learnt from: CR PR: BleedingDev/coursition-effect-experiment#0 File: .rules/schema-validation.mdc:0-0 Timestamp: 2025-08-10T21:48:12.331Z Learning: Applies to **/*.types.ts|**/schema/**/*.ts|**/schemas/**/*.ts|**/*-schema.ts|**/*-schemas.ts|**/branded.ts|**/domain/**/*.ts : Always use `Schema as S` for Effect Schema importssrc/domain/media/subtitle-formats/subtitle-streaming.test.ts (1)
1-21: test(types): validate type import correctnessI’ve verified that there are no remaining “only refers to a value, but is being used as a type” errors in your tests. The
import type { SubtitleItem } from './subtitle-formats.schema'remains valid now that schema types useS.Type<…>.If you decide to rename runtime schemas (for example,
SubtitleItemSchema) to avoid value/type shadowing, please update the corresponding type imports here:
- File: src/domain/media/subtitle-formats/subtitle-streaming.test.ts (lines 1–21)
| ## API Reference | ||
|
|
||
| ### Pipeline Creation | ||
|
|
||
| #### `createPipeline(config?)` | ||
| Creates a new pipeline with optional configuration. | ||
|
|
||
| #### `createArrayPipeline(items, config?)` | ||
| Creates a pipeline that processes an array of subtitle items. | ||
|
|
||
| ### Pipeline Methods | ||
|
|
||
| #### `.stream(generator)` | ||
| Adds a streaming stage to the pipeline. | ||
|
|
||
| #### `.filter(filter)` | ||
| Adds a filter stage to the pipeline. | ||
|
|
||
| #### `.parallelFilter(filter)` | ||
| Adds a parallel filter stage to the pipeline. | ||
|
|
||
| #### `.collector(collector)` | ||
| Adds a collector stage to the pipeline. | ||
|
|
||
| #### `.formatter(formatter)` | ||
| Adds a formatter stage to the pipeline. | ||
|
|
||
| #### `.execute()` | ||
| Executes the pipeline and returns the result. | ||
|
|
||
| ### Pre-built Functions | ||
|
|
||
| #### `processToSrt(items, filters?)` | ||
| Processes subtitles and converts to SRT format. | ||
|
|
||
| #### `processToVtt(items, filters?)` | ||
| Processes subtitles and converts to VTT format. | ||
|
|
||
| #### `processWithConfig(items, filters?, config?)` | ||
| Processes subtitles with custom configuration. | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🛠️ Refactor suggestion
🧩 Analysis chain
docs(subtitles): document new streaming APIs runSubtitleProcessingStream/runSubtitleConversionStream
PR objectives mention new stream-based functions, but this README doesn’t show them. Please add a “Streaming APIs” section with examples for:
- runSubtitleProcessingStream: consuming a generator/async iterator, applying filters, yielding items.
- runSubtitleConversionStream: producing incremental SRT/VTT/text/JSON fragments.
This will align the documentation with the new public capability.
Would you like me to draft a README section with runnable examples for both functions?
🏁 Script executed:
#!/bin/bash
set -e
echo "🔍 Searching for runSubtitleProcessingStream in the codebase…"
rg -n "runSubtitleProcessingStream"
echo "🔍 Searching for runSubtitleConversionStream in the codebase…"
rg -n "runSubtitleConversionStream"Length of output: 1382
docs(subtitles): add Streaming APIs section for new stream-based functions
The README currently documents pipeline methods and pre-built functions but omits the newly introduced runSubtitleProcessingStream and runSubtitleConversionStream. Please add a Streaming APIs section (e.g. immediately after “Pre-built Functions”) that:
- Describes
runSubtitleProcessingStream(items, filters?, config?)
• Consumes a (async) generator, applies filters, and yields processed subtitle items. - Describes
runSubtitleConversionStream(items, filters?, config?)
• Produces incremental output (SRT/VTT/text/JSON fragments) as it processes the stream.
This aligns the docs with the implementations in
- src/domain/media/subtitle-formats/subtitle-converter.ts: lines 351 (
runSubtitleProcessingStream) - src/domain/media/subtitle-formats/subtitle-converter.ts: lines 446 (
runSubtitleConversionStream)
Let me know if you’d like example snippets drafted for both functions.
🧰 Tools
🪛 LanguageTool
[style] ~80-~80: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...peline. #### .parallelFilter(filter) Adds a parallel filter stage to the pipeline...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~83-~83: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...pipeline. #### .collector(collector) Adds a collector stage to the pipeline. ###...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~86-~86: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...pipeline. #### .formatter(formatter) Adds a formatter stage to the pipeline. ###...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~100-~100: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ssWithConfig(items, filters?, config?)` Processes subtitles with custom configuration. #...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.17.2)
66-66: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
69-69: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
74-74: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
77-77: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
80-80: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
83-83: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
86-86: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
89-89: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
94-94: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
97-97: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
100-100: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/README.md around lines 62 to 102, the
README is missing documentation for the new stream-based APIs; add a "Streaming
APIs" section immediately after "Pre-built Functions" that documents
runSubtitleProcessingStream(items, filters?, config?) — note it consumes an
(async) generator, applies filters, and yields processed subtitle items — and
runSubtitleConversionStream(items, filters?, config?) — note it produces
incremental output (SRT/VTT/text/JSON fragments) as it processes the stream;
reference the implementations in
src/domain/media/subtitle-formats/subtitle-converter.ts at lines ~351 and ~446
and keep the descriptions concise and parallel to existing function docs.
| expect(result).toHaveLength(1) // Only speaker 1, no empty subtitles | ||
| expect(result[0]?.start).toBe(500) // Timing offset applied | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
test(expectations): update counts after fixing per-element filtering semantics
Filtering by speaker 1 should return both speaker-1 items (non-empty), not terminate after encountering a speaker-2 item.
- expect(result).toHaveLength(1) // Only speaker 1, no empty subtitles
- expect(result[0]?.start).toBe(500) // Timing offset applied
+ expect(result).toHaveLength(2) // Only speaker 1, no empty subtitles
+ expect(result[0]?.start).toBe(500) // First speaker 1, offset applied
+ expect(result[1]?.start).toBe(5500) // Second speaker 1, offset applied📝 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.
| expect(result).toHaveLength(1) // Only speaker 1, no empty subtitles | |
| expect(result[0]?.start).toBe(500) // Timing offset applied | |
| }) | |
| expect(result).toHaveLength(2) // Only speaker 1, no empty subtitles | |
| expect(result[0]?.start).toBe(500) // First speaker 1, offset applied | |
| expect(result[1]?.start).toBe(5500) // Second speaker 1, offset applied | |
| }) |
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-streaming.test.ts around lines 90
to 92, update the assertions to reflect that filtering by speaker 1 should
return both non-empty speaker-1 items (the filter is per-element and should not
stop at a speaker-2 item). Change the length expectation from 1 to 2 and adjust
the subsequent checks to assert both returned items exist and have the correct
start times (with the timing offset applied) instead of only checking result[0].
| expect(result).toHaveLength(0) // Only speaker 2, no empty subtitles | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
test(expectations): fix parallel pipeline expectation (should be 2 items for speaker 2)
Two items have speaker 2 and are non-empty; prefix doesn’t affect filtering.
- expect(result).toHaveLength(0) // Only speaker 2, no empty subtitles
+ expect(result).toHaveLength(2) // Only speaker 2, no empty subtitles📝 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.
| expect(result).toHaveLength(0) // Only speaker 2, no empty subtitles | |
| }) | |
| expect(result).toHaveLength(2) // Only speaker 2, no empty subtitles | |
| }) |
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-streaming.test.ts around lines 104
to 105, the test incorrectly expects zero results for speaker 2 but there are
two non-empty items with speaker 2; update the assertion to expect 2 items
(e.g., change to toHaveLength(2)) and adjust the inline comment to reflect that
two non-empty subtitles belong to speaker 2.
| expect(result).toHaveLength(1) | ||
| expect(result[0]?.speaker).toBe(1) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
test(expectations): fix filter-by-speaker expectation (should be 2)
There are two speaker-1 items.
- expect(result).toHaveLength(1)
+ expect(result).toHaveLength(2)📝 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.
| expect(result).toHaveLength(1) | |
| expect(result[0]?.speaker).toBe(1) | |
| }) | |
| expect(result).toHaveLength(2) | |
| expect(result[0]?.speaker).toBe(1) | |
| }) |
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-streaming.test.ts around lines 149
to 151, the test assertions expect only one speaker-1 item but there are
actually two; update the expectations to reflect two results
(expect(result).toHaveLength(2)) and assert both entries are speaker 1 (e.g.,
check result[0].speaker and result[1].speaker are 1 or use a loop/array-every
assertion) so the test matches the actual filtered output.
| expect(result).toHaveLength(3) // One empty subtitle removed | ||
| expect(result.every((subtitle) => subtitle.text.trim().length > 0)).toBe( | ||
| true, | ||
| ) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
test(expectations): fix remove-empty expectation (should be 4)
Only a single empty subtitle is removed; others remain.
- expect(result).toHaveLength(3) // One empty subtitle removed
+ expect(result).toHaveLength(4) // One empty subtitle removed📝 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.
| expect(result).toHaveLength(3) // One empty subtitle removed | |
| expect(result.every((subtitle) => subtitle.text.trim().length > 0)).toBe( | |
| true, | |
| ) | |
| }) | |
| expect(result).toHaveLength(4) // One empty subtitle removed | |
| expect(result.every((subtitle) => subtitle.text.trim().length > 0)).toBe( | |
| true, | |
| ) | |
| }) |
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-streaming.test.ts around lines 162
to 166, the test incorrectly expects 3 subtitles after removal even though only
one empty subtitle is removed; update the length expectation to 4 (change
expect(result).toHaveLength(3) to expect(result).toHaveLength(4)) and keep the
existing non-empty-text assertion as-is.
| expect(result).toHaveLength(1) // Only speaker 1, no empty subtitles | ||
| expect(result[0]?.start).toBe(2000) // Timing offset applied | ||
| expect(result[0]?.text).toBe('[PROCESSED] HELLO, WORLD!') // Text transformed | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
test(expectations): fix complex chain expectation (should be 2 for speaker 1)
Both speaker-1 items survive validation and removal; then offset and transforms apply.
- expect(result).toHaveLength(1) // Only speaker 1, no empty subtitles
- expect(result[0]?.start).toBe(2000) // Timing offset applied
- expect(result[0]?.text).toBe('[PROCESSED] HELLO, WORLD!') // Text transformed
+ expect(result).toHaveLength(2) // Only speaker 1, no empty subtitles
+ expect(result[0]?.start).toBe(2000)
+ expect(result[0]?.text).toBe('[PROCESSED] HELLO, WORLD!')
+ expect(result[1]?.start).toBe(7000)
+ expect(result[1]?.text).toBe('[PROCESSED] THIS IS A TEST.')📝 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.
| expect(result).toHaveLength(1) // Only speaker 1, no empty subtitles | |
| expect(result[0]?.start).toBe(2000) // Timing offset applied | |
| expect(result[0]?.text).toBe('[PROCESSED] HELLO, WORLD!') // Text transformed | |
| }) | |
| expect(result).toHaveLength(2) // Only speaker 1, no empty subtitles | |
| expect(result[0]?.start).toBe(2000) | |
| expect(result[0]?.text).toBe('[PROCESSED] HELLO, WORLD!') | |
| expect(result[1]?.start).toBe(7000) | |
| expect(result[1]?.text).toBe('[PROCESSED] THIS IS A TEST.') | |
| }) |
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-streaming.test.ts around lines 248
to 251, the test incorrectly expects only one surviving speaker-1 subtitle;
update the assertions to expect two items (expect(result).toHaveLength(2)) and
add/adjust expectations for the second item (its start time after offset and its
transformed text) so both speaker-1 entries are asserted instead of only one.
There was a problem hiding this comment.
Actionable comments posted: 19
♻️ Duplicate comments (3)
src/domain/media/subtitle-formats/subtitle-filters.ts (2)
225-264: refactor: collapse double-pass array processing into a single passThe current implementation iterates twice (filter + map) with duplicated logic. This is unnecessary and increases complexity.
Apply this diff to make it single-pass while preserving Option handling:
export const applyFiltersToArray = ( subtitles: SubtitleItem[], ...filters: Array< (subtitle: SubtitleItem) => SubtitleItem | Option.Option<SubtitleItem> > ): SubtitleItem[] => { - return subtitles - .filter((subtitle) => { - let current = subtitle - for (const filter of filters) { - const result = filter(current) - if (Option.isOption(result)) { - if (Option.isSome(result)) { - current = result.value - } else { - return false // Filtered out - } - } else { - current = result - } - } - return true - }) - .map((subtitle) => { - let current = subtitle - for (const filter of filters) { - const result = filter(current) - if (Option.isOption(result)) { - if (Option.isSome(result)) { - current = result.value - } else { - return subtitle // Should not happen due to filter above - } - } else { - current = result - } - } - return current - }) + const results: SubtitleItem[] = [] + for (const subtitle of subtitles) { + let current = subtitle + let include = true + for (const filter of filters) { + const result = filter(current) + if (Option.isOption(result)) { + if (Option.isSome(result)) { + current = result.value + } else { + include = false + break + } + } else { + current = result + } + } + if (include) results.push(current) + } + return results }
541-541: fix: return domain error for unsupported formats in saveToFileUse a tagged domain error instead of a generic Error to stay consistent with the error model.
Apply this diff:
- default: - return E.fail(new Error(`Unsupported format: ${format}`)) + default: + return E.fail( + new UnsupportedFormatError({ + format, + supportedFormats: ['json', 'srt', 'vtt', 'plain-text'], + }), + )Additionally, add the import at the top of this file:
import { UnsupportedFormatError } from './subtitle-formats.errors'src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts (1)
1-1: test: import from @effect/vitest for Effect-aligned testsPer repo guidelines, tests should import from @effect/vitest.
Apply this diff:
-import { describe, expect, it } from 'vitest' +import { describe, expect, it } from '@effect/vitest'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/domain/media/subtitle-formats/subtitle-converter-clean.test.ts(1 hunks)src/domain/media/subtitle-formats/subtitle-converter.test.ts(1 hunks)src/domain/media/subtitle-formats/subtitle-converter.ts(1 hunks)src/domain/media/subtitle-formats/subtitle-filters.ts(1 hunks)src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.ts
📄 CodeRabbit Inference Engine (.rules/code-style.mdc)
src/**/*.ts: Always use relative paths for internal imports; never usesrc/aliases for internal imports in TypeScript files.
Use package imports only for external dependencies in TypeScript files.
Avoid obvious comments; never add comments that just repeat what the code is doing. Only add comments for complex business logic that needs explanation.
Use JSDoc for public APIs, not inline comments for obvious operations.
Add comments only for complex business logic, non-obvious workarounds, edge cases, or important context that affects future changes.
UseEffect.Serviceclass extension pattern for services, include dependencies in service definition, and providemakeTestServicefor testing.
UseE.gen(function* () {})for complex flows; yield effects directly without helper functions; useyield*for effects that return values.
UseData.Taggedfor domain errors, create specific error types for different failure cases, and handle errors explicitly withE.catchTags.
Use Effect's logging utilities instead ofconsole.log; include relevant context in log messages and use appropriate log levels.
Add spans for observability usingE.withSpan, include relevant attributes, and use descriptive span names.
Use PascalCase for service classes (e.g.,BlogStore,SearchStore).
Use camelCase for functions and variables (e.g.,getBlogById,blogPost).
Use UPPERCASE for constants (e.g.,TABLE_NAME,DEFAULT_LIMIT).
Use descriptive names for functions and methods that indicate purpose.
Prefix usecases with action (e.g.,createBlogPostUsecase,getBlogByIdUsecase).
Prefix handlers with HTTP method context (e.g.,_handler,handler).
Use Effect Schema for all data validation; define schemas with explicit types and use branded types for domain identifiers.
Use branded types for domain identifiers and create factory functions with validation.
Access configuration through the Effect system, not directly viaprocess.env.
Use pipe syntax for effect composition;...
Files:
src/domain/media/subtitle-formats/subtitle-converter-clean.test.tssrc/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-converter.tssrc/domain/media/subtitle-formats/subtitle-filters.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
src/**/*
📄 CodeRabbit Inference Engine (.rules/code-style.mdc)
Use kebab-case for file names (e.g.,
blog.store.ts,get-blog-by-id.ts).
Files:
src/domain/media/subtitle-formats/subtitle-converter-clean.test.tssrc/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-converter.tssrc/domain/media/subtitle-formats/subtitle-filters.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
src/**/*.test.ts
📄 CodeRabbit Inference Engine (.rules/code-style.mdc)
Always use
@effect/vitestfor Effect-based tests; use appropriate test variants (it.effect,it.scoped,it.live); mock services usingmakeTestService.
Files:
src/domain/media/subtitle-formats/subtitle-converter-clean.test.tssrc/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.rules/effect-service-architecture.mdc)
Always use relative paths for internal imports; never use
src/aliases for internal imports. Use package imports only for external dependencies.
Files:
src/domain/media/subtitle-formats/subtitle-converter-clean.test.tssrc/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-converter.tssrc/domain/media/subtitle-formats/subtitle-filters.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
**/*.test.ts
📄 CodeRabbit Inference Engine (.rules/effect-testing-patterns.mdc)
**/*.test.ts: Always use@effect/vitestfor tests involving Effect-based code; import test functions (describe,expect,it) from@effect/vitest, not regularvitest.
Always use relative paths for internal imports in test files; never usesrc/aliases.
Use package imports for external dependencies in test files.
Test tagged errors with proper Effect patterns (usingE.exitandgetExitError).
Useit.effectfor tests that need TestContext (TestClock, etc.) in Effect-TS test files.
Useit.scopedfor tests involving resources that need proper cleanup in Effect-TS test files.
Useit.livefor tests that need real system time and live services in Effect-TS test files.
UseFetchHttpClientTestfrom@t6c/lib-apifor mocking HTTP responses in API client tests.
Use mock data from domain packages when available in tests.
Create reusable mock objects for complex data structures in tests.
Use proper branded type constructors for mock data in tests.
Declare variables outside generator functions when used inFetchHttpClientTestto prevent scoping issues in tests.
Use.pipe(E.exit)directly on service calls for cleaner error testing in tests; avoid nestedE.exit(E.gen(...))patterns.
UseExit.isFailure(result)to check for failures andgetExitError(result)to extract error details in tests.
Test all public methods of services, including both success and error scenarios, default parameters, edge cases, and validation.
Use appropriate HTTP status codes (200, 201, 404, 500) and match response structure to actual API responses in mock responses for tests.
Use descriptive test names that explain the scenario, following the pattern: 'should [action] [condition]'.
Group related tests in describe blocks by method.
Reuse mock data across tests when possible.
Create typed mock objects with proper constructors for tests.
Always provide required layers in correct order in tests (e.g.,DefaultWithoutDependenciesfor services,MockConfigLayerfor configurat...
Files:
src/domain/media/subtitle-formats/subtitle-converter-clean.test.tssrc/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
🧠 Learnings (24)
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Use proper branded type constructors for mock data in tests.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:45:57.853Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/code-style.mdc:0-0
Timestamp: 2025-08-10T21:45:57.853Z
Learning: Applies to src/**/*.ts : Write pure functions without side effects; use Effect for operations with side effects; keep business logic separate from infrastructure.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-filters.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Use proper Effect error handling patterns in utility functions.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-converter.tssrc/domain/media/subtitle-formats/subtitle-filters.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Use `it.effect` for tests that need TestContext (TestClock, etc.) in Effect-TS test files.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Use Effect-TS patterns for async operations in utility functions.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-converter.tssrc/domain/media/subtitle-formats/subtitle-filters.ts
📚 Learning: 2025-08-10T21:45:57.853Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/code-style.mdc:0-0
Timestamp: 2025-08-10T21:45:57.853Z
Learning: Applies to src/**/*.ts : Access configuration through the Effect system, not directly via `process.env`.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Always use `effect/vitest` for tests involving Effect-based code; import test functions (`describe`, `expect`, `it`) from `effect/vitest`, not regular `vitest`.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
📚 Learning: 2025-08-10T21:45:57.853Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/code-style.mdc:0-0
Timestamp: 2025-08-10T21:45:57.853Z
Learning: Applies to src/**/*.ts : Use `E.gen(function* () {})` for complex flows; yield effects directly without helper functions; use `yield*` for effects that return values.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Log errors within the Effect system using `E.logError` or `E.log`, not `console.log`.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:46:54.925Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-store-patterns.mdc:0-0
Timestamp: 2025-08-10T21:46:54.925Z
Learning: Applies to **/*.store.ts : Store classes should integrate with external services (e.g., Algolia, DynamoDB, S3) using Effect-TS patterns
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Use `yield* new ErrorClass()` in Effect.gen instead of throwing errors.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:45:57.853Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/code-style.mdc:0-0
Timestamp: 2025-08-10T21:45:57.853Z
Learning: Applies to src/**/*.ts : Use Effect Schema for all data validation; define schemas with explicit types and use branded types for domain identifiers.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.ts
📚 Learning: 2025-08-10T21:48:12.331Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/schema-validation.mdc:0-0
Timestamp: 2025-08-10T21:48:12.331Z
Learning: Applies to **/*.types.ts|**/schema/**/*.ts|**/schemas/**/*.ts|**/*-schema.ts|**/*-schemas.ts|**/branded.ts|**/domain/**/*.ts : Always use `Schema as S` for Effect Schema imports
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.ts
📚 Learning: 2025-08-10T21:46:10.977Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/cursor-rule-maintainer.mdc:0-0
Timestamp: 2025-08-10T21:46:10.977Z
Learning: Applies to .cursor/rules/schema-validation.mdc : Add new Effect Schema patterns, validation improvements, branded type patterns, and error handling for schemas to `schema-validation.mdc`
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.ts
📚 Learning: 2025-08-10T21:48:12.331Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/schema-validation.mdc:0-0
Timestamp: 2025-08-10T21:48:12.331Z
Learning: Applies to **/schema/**/*.ts|**/schemas/**/*.ts|**/*-schema.ts|**/*-schemas.ts : Test schema validation with valid and invalid inputs using effect/vitest and Effect-based test patterns
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
📚 Learning: 2025-08-10T21:47:41.747Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-usecase-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:41.747Z
Learning: Applies to **/*-usecase.ts : UPDATE/DELETE usecases must use E.orDie for unexpected errors and use only simple error logging with E.tapError(E.logError).
Applied to files:
src/domain/media/subtitle-formats/subtitle-filters.ts
📚 Learning: 2025-08-10T21:45:57.853Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/code-style.mdc:0-0
Timestamp: 2025-08-10T21:45:57.853Z
Learning: Applies to src/**/*.ts : Use `Data.Tagged` for domain errors, create specific error types for different failure cases, and handle errors explicitly with `E.catchTags`.
Applied to files:
src/domain/media/subtitle-formats/subtitle-filters.ts
📚 Learning: 2025-08-10T21:45:41.119Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/api-client-testing.mdc:0-0
Timestamp: 2025-08-10T21:45:41.119Z
Learning: Applies to **/*api-client.test.ts : API client test files must import { describe, expect, it } from 'effect/vitest', { FetchHttpClientTest, getExitError } from 't6c/lib-api', { Effect as E, Exit } from 'effect', and { MockConfigLayer } from '../config.js'.
Applied to files:
src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
📚 Learning: 2025-08-10T21:45:57.853Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/code-style.mdc:0-0
Timestamp: 2025-08-10T21:45:57.853Z
Learning: Applies to src/**/*.test.ts : Always use `effect/vitest` for Effect-based tests; use appropriate test variants (`it.effect`, `it.scoped`, `it.live`); mock services using `makeTestService`.
Applied to files:
src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
📚 Learning: 2025-08-10T21:45:41.119Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/api-client-testing.mdc:0-0
Timestamp: 2025-08-10T21:45:41.119Z
Learning: Applies to **/*api-client.test.ts : API client test files should follow the standard test structure: use 'it.effect' with an Effect generator, provide layers in the correct order, and assert results using 'expect'.
Applied to files:
src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Use `it.live` for tests that need real system time and live services in Effect-TS test files.
Applied to files:
src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Use `it.scoped` for tests involving resources that need proper cleanup in Effect-TS test files.
Applied to files:
src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
📚 Learning: 2025-08-10T21:46:38.101Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-service-architecture.mdc:0-0
Timestamp: 2025-08-10T21:46:38.101Z
Learning: Applies to **/stores/*-store.test.{ts,js} : Store testing should use Effect-TS testing utilities (e.g., `effect/vitest`), provide mock implementations for dependencies, and test store methods in isolation.
Applied to files:
src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Test tagged errors with proper Effect patterns (using `E.exit` and `getExitError`).
Applied to files:
src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
🧬 Code Graph Analysis (5)
src/domain/media/subtitle-formats/subtitle-converter-clean.test.ts (3)
src/domain/media/subtitle-formats/subtitle-converter.ts (6)
SubtitleItem(740-740)validateSubtitleData(106-150)processSubtitles(304-343)SubtitleConverterLive(659-737)runSubtitleProcessingStream(385-393)runSubtitleConversionStream(480-487)src/domain/media/subtitle-formats/subtitle-formats.errors.ts (2)
UnsupportedFormatError(43-48)InvalidTimingError(50-52)src/domain/media/subtitle-formats/subtitle-filters.ts (6)
replaceText(19-37)addTimingOffset(45-51)addPrefix(84-89)filterBySpeaker(59-62)applyFiltersToArray(225-264)streamSubtitles(273-302)
src/domain/media/subtitle-formats/subtitle-converter.test.ts (4)
src/domain/media/subtitle-formats/subtitle-converter.ts (7)
SubtitleItem(740-740)validateSubtitleData(106-150)processSubtitles(304-343)SubtitleConverterLive(659-737)runSubtitleProcessingStream(385-393)runSubtitleConversionStream(480-487)addSpeakerInfo(187-198)src/domain/media/subtitle-formats/subtitle-formats.schema.ts (2)
SubtitleItem(7-16)SubtitleItem(64-64)src/domain/media/subtitle-formats/subtitle-filters.ts (6)
applyFiltersToArray(225-264)replaceText(19-37)addTimingOffset(45-51)filterBySpeaker(59-62)addPrefix(84-89)streamSubtitles(273-302)src/domain/media/subtitle-formats/subtitle-formats.errors.ts (2)
UnsupportedFormatError(43-48)InvalidTimingError(50-52)
src/domain/media/subtitle-formats/subtitle-converter.ts (2)
src/domain/media/subtitle-formats/subtitle-formats.errors.ts (4)
InvalidSubtitleDataError(37-41)InvalidTimingError(50-52)UnsupportedFormatError(43-48)ConversionError(54-57)src/domain/media/subtitle-formats/subtitle-formats.schema.ts (11)
SubtitleJson(21-21)SubtitleJson(65-65)SubtitleItem(7-16)SubtitleItem(64-64)ConversionOptions(31-42)ConversionOptions(67-67)SubtitleFormat(26-26)SubtitleFormat(66-66)SubtitleConversionResultSchema(54-57)MultipleFormatResult(59-61)MultipleFormatResult(68-70)
src/domain/media/subtitle-formats/subtitle-filters.ts (5)
src/domain/media/subtitle-formats/subtitle-converter.ts (3)
SubtitleItem(740-740)formatTimeSrt(495-501)formatTimeVtt(509-515)src/domain/media/subtitle-formats/subtitle-formats.schema.ts (2)
SubtitleItem(7-16)SubtitleItem(64-64)src/domain/media/subtitle-formats/subtitle-pipeline-simple.ts (3)
filter(169-172)stream(161-164)applyFilters(79-93)src/steps/media/persist-subtitles.step.ts (1)
subtitlesJson(3-14)src/usecases/media/transcribe-workflow.usecase.ts (2)
transcribeMediaStep(51-52)persistSubtitlesStep(55-56)
src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts (3)
src/domain/media/subtitle-formats/subtitle-formats.schema.ts (2)
SubtitleItem(7-16)SubtitleItem(64-64)src/domain/media/subtitle-formats/subtitle-pipeline-simple.ts (13)
createPipeline(266-268)SubtitlePipeline(144-257)createArrayPipeline(273-279)PipelineConfig(43-47)createCollector(121-128)applyFilters(79-93)formatToSrt(288-306)formatToVtt(311-324)formatToPlainText(329-331)formatToJson(336-338)processToSrt(373-382)processToVtt(387-396)processWithConfig(401-415)src/domain/media/subtitle-formats/subtitle-filters.ts (7)
toUpperCase(152-155)applyFilters(348-373)filterBySpeakers(70-76)addTimingOffset(45-51)transformText(140-145)capitalize(172-175)validateSubtitle(202-216)
🔇 Additional comments (2)
src/domain/media/subtitle-formats/subtitle-filters.ts (1)
509-515: docs: reverseStream returns Effect, not a Stream transformThe JSDoc says “returns Stream transformation function” but the function returns Effect<Stream<...>>. Either adjust the docstring or refactor to return a Stream directly (e.g., Stream.fromIterableEffect(...)).
Would you prefer we change the signature to return a Stream (no Effect), or correct the JSDoc to reflect Effect?
src/domain/media/subtitle-formats/subtitle-converter-clean.test.ts (1)
1-1127: test: solid, assertion-focused suite without noisy logsThis file aligns well with the guidelines (uses @effect/vitest, avoids console logs, asserts structure and content across formats and streams). Nicely done.
| `# File size: ${new Blob([content]).size} bytes`, | ||
| ].join('\n') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
test: avoid Blob in Node test environments; use Buffer.byteLength
Blob may not be available or behaves differently across environments. Buffer.byteLength is stable in Node.
Apply this diff:
- `# File size: ${new Blob([content]).size} bytes`,
+ `# File size: ${Buffer.byteLength(content, 'utf8')} bytes`,📝 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.
| `# File size: ${new Blob([content]).size} bytes`, | |
| ].join('\n') | |
| `# File size: ${Buffer.byteLength(content, 'utf8')} bytes`, | |
| ].join('\n') |
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-converter.test.ts around lines
616-617, the test uses new Blob([content]).size to compute file size which is
flaky in Node test environments; replace that expression with
Buffer.byteLength(content, 'utf8') (or Buffer.from(content).length) so the test
uses Node-stable byte length; update the string interpolation to use
Buffer.byteLength(content, 'utf8') and run tests to verify.
| SubtitleConverterLive.convert(subtitles as SubtitleItem[], 'json'), | ||
| ), | ||
| E.catchAll((error) => E.succeed(error)), | ||
| ) | ||
|
|
||
| // The first validation error will be InvalidTimingError for negative start time | ||
| expect(result).toBeInstanceOf(InvalidTimingError) | ||
| }), | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
test: prefer Effect Exit pattern for error assertions
Using E.exit helps assert tagged errors without side effects or manual catchAll branches.
Example:
const exit = yield* SubtitleConverterLive.convert(sampleSubtitles, 'unsupported' as never).pipe(E.exit)
expect(Exit.isFailure(exit)).toBe(true)
expect(getExitError(exit)).toBeInstanceOf(UnsupportedFormatError)Apply similarly for invalid timing scenarios to assert InvalidTimingError.
Also applies to: 1133-1141
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-converter.test.ts around lines
1333–1341 (and similarly at 1133–1141), the tests use E.catchAll/E.succeed to
get errors instead of the Effect Exit pattern; update each test to call
.pipe(E.exit) (or yield* ... .pipe(E.exit)), assert Exit.isFailure(exit) is
true, and then use getExitError(exit) to assert the specific error class (e.g.,
InvalidTimingError or UnsupportedFormatError) so you assert tagged errors
without side effects or manual catchAll branches.
| export const filterByDuration = | ||
| (minDuration: number, maxDuration: number) => | ||
| (subtitle: SubtitleItem): Option.Option<SubtitleItem> => { | ||
| const duration = subtitle.end - subtitle.start | ||
| return duration >= minDuration && duration <= maxDuration | ||
| ? Option.some(subtitle) | ||
| : Option.none() | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
feat: validate duration bounds in filterByDuration
Guard against min > max to avoid surprising “always none” behaviour.
Apply this diff:
export const filterByDuration =
(minDuration: number, maxDuration: number) =>
(subtitle: SubtitleItem): Option.Option<SubtitleItem> => {
+ if (minDuration > maxDuration) {
+ const [min, max] = [maxDuration, minDuration]
+ const duration = subtitle.end - subtitle.start
+ return duration >= min && duration <= max ? Option.some(subtitle) : Option.none()
+ }
const duration = subtitle.end - subtitle.start
return duration >= minDuration && duration <= maxDuration
? Option.some(subtitle)
: Option.none()
}📝 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.
| export const filterByDuration = | |
| (minDuration: number, maxDuration: number) => | |
| (subtitle: SubtitleItem): Option.Option<SubtitleItem> => { | |
| const duration = subtitle.end - subtitle.start | |
| return duration >= minDuration && duration <= maxDuration | |
| ? Option.some(subtitle) | |
| : Option.none() | |
| } | |
| export const filterByDuration = | |
| (minDuration: number, maxDuration: number) => | |
| (subtitle: SubtitleItem): Option.Option<SubtitleItem> => { | |
| if (minDuration > maxDuration) { | |
| const [min, max] = [maxDuration, minDuration] | |
| const duration = subtitle.end - subtitle.start | |
| return duration >= min && duration <= max ? Option.some(subtitle) : Option.none() | |
| } | |
| const duration = subtitle.end - subtitle.start | |
| return duration >= minDuration && duration <= maxDuration | |
| ? Option.some(subtitle) | |
| : Option.none() | |
| } |
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-filters.ts around lines 111 to
118, the filterByDuration function doesn't validate the min/max bounds so
passing minDuration > maxDuration makes it always return none; add a guard at
the start that checks if minDuration > maxDuration and throw a clear Error (e.g.
"filterByDuration: minDuration must be <= maxDuration") to surface invalid
input, then proceed with the existing duration calculation and Option return.
| export const applyFilter = | ||
| <T extends SubtitleItem>( | ||
| filter: (subtitle: SubtitleItem) => T | Option.Option<T>, | ||
| ) => | ||
| (stream: Stream.Stream<SubtitleItem, never, never>) => | ||
| stream.pipe( | ||
| Stream.mapEffect((subtitle) => { | ||
| const result = filter(subtitle) | ||
| if (Option.isOption(result)) { | ||
| return Option.isSome(result) | ||
| ? E.succeed(result.value) | ||
| : E.fail('filtered') | ||
| } | ||
| return E.succeed(result) | ||
| }), | ||
| Stream.catchAll(() => Stream.empty), | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
refactor: avoid string-sentinel errors in stream filters; use filterMap or a tagged error
Returning E.fail('filtered') then catching-all hides other errors and loses type-safety. Either:
- Use Stream.filterMap to keep everything in the Option domain, or
- Fail with a specific Tagged error (e.g., FilteredOutError) and catch only that tag.
Example using filterMap for applyFilter:
return stream.pipe(
Stream.filterMap((subtitle) => {
const result = filter(subtitle)
return Option.isOption(result) ? result : Option.some(result)
})
)And for applyFilters, perform the same within the mapping function by building a final Option and filterMap it.
Also applies to: 348-374
| const formatTimeSrt = (ms: number): string => { | ||
| const hours = Math.floor(ms / 3600000) | ||
| const minutes = Math.floor((ms % 3600000) / 60000) | ||
| const seconds = Math.floor((ms % 60000) / 1000) | ||
| const milliseconds = ms % 1000 | ||
| return `${hours.toString().padStart(2, '0')}:${minutes.toString().padStart(2, '0')}:${seconds.toString().padStart(2, '0')},${milliseconds.toString().padStart(3, '0')}` | ||
| } | ||
|
|
||
| /** | ||
| * Formats time in milliseconds to VTT format (HH:MM:SS.mmm) | ||
| */ | ||
| const formatTimeVtt = (ms: number): string => { | ||
| const hours = Math.floor(ms / 3600000) | ||
| const minutes = Math.floor((ms % 3600000) / 60000) | ||
| const seconds = Math.floor((ms % 60000) / 1000) | ||
| const milliseconds = ms % 1000 | ||
| return `${hours.toString().padStart(2, '0')}:${minutes.toString().padStart(2, '0')}:${seconds.toString().padStart(2, '0')}.${milliseconds.toString().padStart(3, '0')}` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
refactor: deduplicate time-formatting; import shared helpers
Time formatting logic is duplicated here and in subtitle-converter.ts. Prefer a single source of truth.
Apply this diff to remove the local implementations:
-/**
- * Formats time in milliseconds to SRT format (HH:MM:SS,mmm)
- */
-const formatTimeSrt = (ms: number): string => {
- const hours = Math.floor(ms / 3600000)
- const minutes = Math.floor((ms % 3600000) / 60000)
- const seconds = Math.floor((ms % 60000) / 1000)
- const milliseconds = ms % 1000
- return `${hours.toString().padStart(2, '0')}:${minutes.toString().padStart(2, '0')}:${seconds.toString().padStart(2, '0')},${milliseconds.toString().padStart(3, '0')}`
-}
-
-/**
- * Formats time in milliseconds to VTT format (HH:MM:SS.mmm)
- */
-const formatTimeVtt = (ms: number): string => {
- const hours = Math.floor(ms / 3600000)
- const minutes = Math.floor((ms % 3600000) / 60000)
- const seconds = Math.floor((ms % 60000) / 1000)
- const milliseconds = ms % 1000
- return `${hours.toString().padStart(2, '0')}:${minutes.toString().padStart(2, '0')}:${seconds.toString().padStart(2, '0')}.${milliseconds.toString().padStart(3, '0')}`
-}Then add this import near the top of the file:
import { formatTimeSrt, formatTimeVtt } from './subtitle-converter'🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-filters.ts around lines 612 to
629, the local implementations of formatTimeSrt and formatTimeVtt duplicate
logic found in subtitle-converter.ts; remove these two local functions and
replace them by importing the shared helpers. At the top of the file add an
import: import { formatTimeSrt, formatTimeVtt } from './subtitle-converter';
ensure any local references remain unchanged and that the file compiles after
removing the duplicated functions.
| describe('Performance Tests', () => { | ||
| it('should handle large datasets efficiently', () => { | ||
| const largeDataset: SubtitleItem[] = [] | ||
| for (let i = 0; i < 100; i++) { | ||
| largeDataset.push({ | ||
| start: i * 1000, | ||
| end: (i + 1) * 1000, | ||
| text: `Subtitle ${i + 1}`, | ||
| speaker: (i % 3) + 1, | ||
| }) | ||
| } | ||
|
|
||
| const startTime = Date.now() | ||
| const result = createArrayPipeline(largeDataset) | ||
| .filter( | ||
| applyFilters(validateSubtitle, toUpperCase, addPrefix('[PROCESSED]')), | ||
| ) | ||
| .collector(createCollector()) | ||
| .formatter(formatToSrt) | ||
| .execute() | ||
| const endTime = Date.now() | ||
|
|
||
| expect(result).toBeInstanceOf(Array) | ||
| expect((result as string[]).length).toBeGreaterThan(0) | ||
| expect(endTime - startTime).toBeLessThan(1000) // Should complete within 1 second | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
test: avoid time-based performance assertions
Using Date.now in tests is brittle and environment-dependent. Prefer asserting on functional outcomes or using fake timers.
Consider removing the duration assertion or relaxing it behind a much higher threshold, or use a benchmark outside the unit test suite.
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts around
lines 332 to 357, the test uses a real time measurement (Date.now) and asserts
the duration is under 1 second which is brittle; remove the timing assertion and
keep only functional assertions (result is an array and has length), or if you
must assert performance move this to a dedicated benchmark/integration test or
use fake timers and deterministic workload; update the test to either drop
expect(endTime - startTime).toBeLessThan(1000) or replace it with a
non-time-based check or a much higher, documented threshold and/or mark the test
as flaky/slow so it doesn’t run in regular unit test runs.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (12)
src/domain/media/subtitle-formats/subtitle-converter.test.ts (2)
611-617: test: avoid Blob in Node/Vitest; prefer Buffer.byteLength for sizeUse Buffer.byteLength for stable sizing across Node test environments. This was previously raised; keeping the replacement consistent.
- `# File size: ${new Blob([content]).size} bytes`, + `# File size: ${Buffer.byteLength(content, 'utf8')} bytes`,
1130-1138: test: prefer Exit pattern for unsupported format errorAssert UnsupportedFormatError using Exit + Cause, avoiding catchAll on the entire generator.
- it.effect('should reject unsupported format', () => - E.gen(function* () { - const result = yield* SubtitleConverterLive.convert( - sampleSubtitles, - 'unsupported' as never, - ) - expect(result).toBeInstanceOf(UnsupportedFormatError) - }).pipe(E.catchAll(E.succeed)), - ) + it.effect('should reject unsupported format', () => + E.gen(function* () { + const exit = yield* SubtitleConverterLive.convert( + sampleSubtitles, + 'unsupported' as never, + ).pipe(E.exit) + expect(Exit.isFailure(exit)).toBe(true) + if (Exit.isFailure(exit)) { + const errorOpt = Cause.failureOption(exit.cause) + expect(Option.isSome(errorOpt)).toBe(true) + if (Option.isSome(errorOpt)) { + expect(errorOpt.value).toBeInstanceOf(UnsupportedFormatError) + } + } + }), + )src/domain/media/subtitle-formats/subtitle-filters.ts (7)
70-76: Use Set for O(1) speaker lookup performanceWhen dealing with potentially large speaker ID arrays, using
Set.has()provides O(1) lookup complexity compared toArray.includes()O(n) complexity.export const filterBySpeakers = (speakerIds: number[]) => - (subtitle: SubtitleItem): Option.Option<SubtitleItem> => - typeof subtitle.speaker === 'number' && - speakerIds.includes(subtitle.speaker) - ? Option.some(subtitle) - : Option.none() + (subtitle: SubtitleItem): Option.Option<SubtitleItem> => { + const speakerSet = new Set(speakerIds) + return typeof subtitle.speaker === 'number' && + speakerSet.has(subtitle.speaker) + ? Option.some(subtitle) + : Option.none() + }
84-102: Prevent double spaces in prefix/suffix operationsThe current implementation may introduce double spaces if the prefix/suffix already contains trailing/leading whitespace.
export const addPrefix = (prefix: string) => (subtitle: SubtitleItem): SubtitleItem => ({ ...subtitle, - text: `${prefix} ${subtitle.text}`, + text: prefix.endsWith(' ') ? `${prefix}${subtitle.text}` : `${prefix} ${subtitle.text}`, }) export const addSuffix = (suffix: string) => (subtitle: SubtitleItem): SubtitleItem => ({ ...subtitle, - text: `${subtitle.text} ${suffix}`, + text: suffix.startsWith(' ') ? `${subtitle.text}${suffix}` : `${subtitle.text} ${suffix}`, })
111-118: Add validation for duration boundsConsider validating that
minDuration <= maxDurationto prevent unexpected filtering behaviour.export const filterByDuration = (minDuration: number, maxDuration: number) => (subtitle: SubtitleItem): Option.Option<SubtitleItem> => { + if (minDuration > maxDuration) { + throw new Error('filterByDuration: minDuration must be <= maxDuration') + } const duration = subtitle.end - subtitle.start return duration >= minDuration && duration <= maxDuration ? Option.some(subtitle) : Option.none() }
225-237: Refactor to single-pass iteration for efficiencyThe function currently iterates through the array twice with identical logic. This can be optimised to a single pass.
export const applyFiltersToArray = ( subtitles: SubtitleItem[], ...filters: Array< (subtitle: SubtitleItem) => SubtitleItem | Option.Option<SubtitleItem> > ): SubtitleItem[] => { - return subtitles - .filter((subtitle) => Option.isSome(applyFiltersToItem(subtitle, filters))) - .map((subtitle) => { - const result = applyFiltersToItem(subtitle, filters) - return Option.isSome(result) ? result.value : subtitle - }) + const results: SubtitleItem[] = [] + for (const subtitle of subtitles) { + const result = applyFiltersToItem(subtitle, filters) + if (Option.isSome(result)) { + results.push(result.value) + } + } + return results }
281-297: Use type-safe filtering with Stream.filterMapUsing string-sentinel errors ('filtered') loses type safety. Consider using
Stream.filterMapto keep filtering within the Option domain.export const applyFilter = <T extends SubtitleItem>( filter: (subtitle: SubtitleItem) => T | Option.Option<T>, ) => (stream: Stream.Stream<SubtitleItem, never, never>) => stream.pipe( - Stream.mapEffect((subtitle) => { + Stream.filterMap((subtitle) => { const result = filter(subtitle) - if (Option.isOption(result)) { - return Option.isSome(result) - ? E.succeed(result.value) - : E.fail('filtered') - } - return E.succeed(result) - }), - Stream.catchAll(() => Stream.empty), + return Option.isOption(result) ? result : Option.some(result) + }) )Apply similar refactoring to
applyFilters:export const applyFilters = ( ...filters: Array< (subtitle: SubtitleItem) => SubtitleItem | Option.Option<SubtitleItem> > ) => (stream: Stream.Stream<SubtitleItem, never, never>) => stream.pipe( - Stream.mapEffect((subtitle) => { - const result = applyFiltersToItem(subtitle, filters) - return Option.isSome(result) - ? E.succeed(result.value) - : E.fail('filtered') - }), - Stream.catchAll(() => Stream.empty), + Stream.filterMap((subtitle) => applyFiltersToItem(subtitle, filters)) )Also applies to: 305-320
488-508: Use domain-specific error types for better error handlingReplace generic Error with the domain-specific
UnsupportedFormatErrorfor consistency with the codebase error handling patterns.+import { UnsupportedFormatError } from './subtitle-formats.errors' export const saveToFile = (format: 'json' | 'srt' | 'vtt' | 'plain-text') => (stream: Stream.Stream<SubtitleItem, never, never>) => stream.pipe( Stream.runCollect, E.map((chunk) => Array.from(chunk)), E.flatMap((subtitles) => { switch (format) { case 'json': return E.succeed(JSON.stringify(subtitles, null, 2)) case 'srt': return E.succeed(convertToSrtFormat(subtitles)) case 'vtt': return E.succeed(convertToVttFormat(subtitles)) case 'plain-text': return E.succeed(convertToPlainTextFormat(subtitles)) default: - return E.fail(new Error(`Unsupported format: ${format}`)) + return E.fail(new UnsupportedFormatError({ + format: format as string, + supportedFormats: ['json', 'srt', 'vtt', 'plain-text'] + })) } }), )
573-590: Remove duplicate time formatting functionsThese time formatting functions are duplicated across multiple files. Import them from a shared module to maintain DRY principles.
+import { formatTimeSrt, formatTimeVtt } from './subtitle-converter' + // ... rest of the file ... -/** - * Formats time in milliseconds to SRT format (HH:MM:SS,mmm) - */ -const formatTimeSrt = (ms: number): string => { - const hours = Math.floor(ms / 3600000) - const minutes = Math.floor((ms % 3600000) / 60000) - const seconds = Math.floor((ms % 60000) / 1000) - const milliseconds = ms % 1000 - return `${hours.toString().padStart(2, '0')}:${minutes.toString().padStart(2, '0')}:${seconds.toString().padStart(2, '0')},${milliseconds.toString().padStart(3, '0')}` -} - -/** - * Formats time in milliseconds to VTT format (HH:MM:SS.mmm) - */ -const formatTimeVtt = (ms: number): string => { - const hours = Math.floor(ms / 3600000) - const minutes = Math.floor((ms % 3600000) / 60000) - const seconds = Math.floor((ms % 60000) / 1000) - const milliseconds = ms % 1000 - return `${hours.toString().padStart(2, '0')}:${minutes.toString().padStart(2, '0')}:${seconds.toString().padStart(2, '0')}.${milliseconds.toString().padStart(3, '0')}` -}src/domain/media/subtitle-formats/subtitle-pipeline-simple.ts (3)
98-116: Avoid E.runSync in pure functions - return Effect insteadUsing
E.runSynccan throw exceptions if the Effect fails. Return an Effect to allow proper error handling by the caller.export const processParallel = (filter: SubtitleFilter) => - (items: SubtitleItem[]): SubtitleItem[] => { + (items: SubtitleItem[]): E.Effect<SubtitleItem[], never, never> => { const program = E.all( items.map((item) => { const result = applySingleFilter(filter)(item) if (Option.isSome(result)) { return E.succeed(result.value) } return E.succeed(null) }), ).pipe( E.map((results) => results.filter((item): item is SubtitleItem => item !== null), ), ) - return E.runSync(program) + return program }Then update the
executemethod to handle the Effect properly:const handleFilter = ( stage: FilterStage, items: SubtitleItem[], ): SubtitleItem[] => { if (this.config.parallelProcessing) { - return processParallel(stage.filter)(items) + return E.runSync(processParallel(stage.filter)(items)) } // ... rest of the method }
287-290: Remove unused exhaustive check in default caseThe default case should either throw an error or use TypeScript's
satisfiesoperator for exhaustiveness checking.default: { - // This should never happen due to TypeScript's exhaustive checking - break + // TypeScript's exhaustive checking ensures this is never reached + stage satisfies never + break }
386-403: Extract duplicate time formatting to shared utilitiesThese time formatting functions duplicate logic from subtitle-converter.ts. Import them instead to maintain a single source of truth.
+import { formatTimeSrt, formatTimeVtt } from './subtitle-converter' + // ... rest of the file ... -/** - * Formats time in milliseconds to SRT format (HH:MM:SS,mmm) - */ -const formatTimeSrt = (ms: number): string => { - const hours = Math.floor(ms / 3600000) - const minutes = Math.floor((ms % 3600000) / 60000) - const seconds = Math.floor((ms % 60000) / 1000) - const milliseconds = ms % 1000 - return `${hours.toString().padStart(2, '0')}:${minutes.toString().padStart(2, '0')}:${seconds.toString().padStart(2, '0')},${milliseconds.toString().padStart(3, '0')}` -} - -/** - * Formats time in milliseconds to VTT format (HH:MM:SS.mmm) - */ -const formatTimeVtt = (ms: number): string => { - const hours = Math.floor(ms / 3600000) - const minutes = Math.floor((ms % 3600000) / 60000) - const seconds = Math.floor((ms % 60000) / 1000) - const milliseconds = ms % 1000 - return `${hours.toString().padStart(2, '0')}:${minutes.toString().padStart(2, '0')}:${seconds.toString().padStart(2, '0')}.${milliseconds.toString().padStart(3, '0')}` -}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/domain/media/subtitle-formats/subtitle-converter.test.ts(1 hunks)src/domain/media/subtitle-formats/subtitle-filters.ts(1 hunks)src/domain/media/subtitle-formats/subtitle-pipeline-simple.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.ts
📄 CodeRabbit Inference Engine (.rules/code-style.mdc)
src/**/*.ts: Always use relative paths for internal imports; never usesrc/aliases for internal imports in TypeScript files.
Use package imports only for external dependencies in TypeScript files.
Avoid obvious comments; never add comments that just repeat what the code is doing. Only add comments for complex business logic that needs explanation.
Use JSDoc for public APIs, not inline comments for obvious operations.
Add comments only for complex business logic, non-obvious workarounds, edge cases, or important context that affects future changes.
UseEffect.Serviceclass extension pattern for services, include dependencies in service definition, and providemakeTestServicefor testing.
UseE.gen(function* () {})for complex flows; yield effects directly without helper functions; useyield*for effects that return values.
UseData.Taggedfor domain errors, create specific error types for different failure cases, and handle errors explicitly withE.catchTags.
Use Effect's logging utilities instead ofconsole.log; include relevant context in log messages and use appropriate log levels.
Add spans for observability usingE.withSpan, include relevant attributes, and use descriptive span names.
Use PascalCase for service classes (e.g.,BlogStore,SearchStore).
Use camelCase for functions and variables (e.g.,getBlogById,blogPost).
Use UPPERCASE for constants (e.g.,TABLE_NAME,DEFAULT_LIMIT).
Use descriptive names for functions and methods that indicate purpose.
Prefix usecases with action (e.g.,createBlogPostUsecase,getBlogByIdUsecase).
Prefix handlers with HTTP method context (e.g.,_handler,handler).
Use Effect Schema for all data validation; define schemas with explicit types and use branded types for domain identifiers.
Use branded types for domain identifiers and create factory functions with validation.
Access configuration through the Effect system, not directly viaprocess.env.
Use pipe syntax for effect composition;...
Files:
src/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-filters.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.ts
src/**/*
📄 CodeRabbit Inference Engine (.rules/code-style.mdc)
Use kebab-case for file names (e.g.,
blog.store.ts,get-blog-by-id.ts).
Files:
src/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-filters.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.ts
src/**/*.test.ts
📄 CodeRabbit Inference Engine (.rules/code-style.mdc)
Always use
@effect/vitestfor Effect-based tests; use appropriate test variants (it.effect,it.scoped,it.live); mock services usingmakeTestService.
Files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.rules/effect-service-architecture.mdc)
Always use relative paths for internal imports; never use
src/aliases for internal imports. Use package imports only for external dependencies.
Files:
src/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-filters.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.ts
**/*.test.ts
📄 CodeRabbit Inference Engine (.rules/effect-testing-patterns.mdc)
**/*.test.ts: Always use@effect/vitestfor tests involving Effect-based code; import test functions (describe,expect,it) from@effect/vitest, not regularvitest.
Always use relative paths for internal imports in test files; never usesrc/aliases.
Use package imports for external dependencies in test files.
Test tagged errors with proper Effect patterns (usingE.exitandgetExitError).
Useit.effectfor tests that need TestContext (TestClock, etc.) in Effect-TS test files.
Useit.scopedfor tests involving resources that need proper cleanup in Effect-TS test files.
Useit.livefor tests that need real system time and live services in Effect-TS test files.
UseFetchHttpClientTestfrom@t6c/lib-apifor mocking HTTP responses in API client tests.
Use mock data from domain packages when available in tests.
Create reusable mock objects for complex data structures in tests.
Use proper branded type constructors for mock data in tests.
Declare variables outside generator functions when used inFetchHttpClientTestto prevent scoping issues in tests.
Use.pipe(E.exit)directly on service calls for cleaner error testing in tests; avoid nestedE.exit(E.gen(...))patterns.
UseExit.isFailure(result)to check for failures andgetExitError(result)to extract error details in tests.
Test all public methods of services, including both success and error scenarios, default parameters, edge cases, and validation.
Use appropriate HTTP status codes (200, 201, 404, 500) and match response structure to actual API responses in mock responses for tests.
Use descriptive test names that explain the scenario, following the pattern: 'should [action] [condition]'.
Group related tests in describe blocks by method.
Reuse mock data across tests when possible.
Create typed mock objects with proper constructors for tests.
Always provide required layers in correct order in tests (e.g.,DefaultWithoutDependenciesfor services,MockConfigLayerfor configurat...
Files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
🧠 Learnings (28)
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Test all public methods of services, including both success and error scenarios, default parameters, edge cases, and validation.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Use proper branded type constructors for mock data in tests.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:45:57.853Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/code-style.mdc:0-0
Timestamp: 2025-08-10T21:45:57.853Z
Learning: Applies to src/**/*.ts : Write pure functions without side effects; use Effect for operations with side effects; keep business logic separate from infrastructure.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-filters.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Use proper Effect error handling patterns in utility functions.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-filters.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Use `it.effect` for tests that need TestContext (TestClock, etc.) in Effect-TS test files.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Use Effect-TS patterns for async operations in utility functions.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-filters.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.ts
📚 Learning: 2025-08-10T21:45:57.853Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/code-style.mdc:0-0
Timestamp: 2025-08-10T21:45:57.853Z
Learning: Applies to src/**/*.ts : Access configuration through the Effect system, not directly via `process.env`.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Always use `effect/vitest` for tests involving Effect-based code; import test functions (`describe`, `expect`, `it`) from `effect/vitest`, not regular `vitest`.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:45:57.853Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/code-style.mdc:0-0
Timestamp: 2025-08-10T21:45:57.853Z
Learning: Applies to src/**/*.ts : Use `E.gen(function* () {})` for complex flows; yield effects directly without helper functions; use `yield*` for effects that return values.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Log errors within the Effect system using `E.logError` or `E.log`, not `console.log`.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:46:54.925Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-store-patterns.mdc:0-0
Timestamp: 2025-08-10T21:46:54.925Z
Learning: Applies to **/*.store.ts : Store classes should integrate with external services (e.g., Algolia, DynamoDB, S3) using Effect-TS patterns
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Use `yield* new ErrorClass()` in Effect.gen instead of throwing errors.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Use `.pipe(E.exit)` directly on service calls for cleaner error testing in tests; avoid nested `E.exit(E.gen(...))` patterns.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:45:41.119Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/api-client-testing.mdc:0-0
Timestamp: 2025-08-10T21:45:41.119Z
Learning: Applies to **/*api-client.test.ts : Use the clean exit pattern for error testing in API client test files: apply '.pipe(E.exit)' directly to service calls, avoid nested 'E.exit(E.gen(...))', and keep layer provision at the test level.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:45:41.119Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/api-client-testing.mdc:0-0
Timestamp: 2025-08-10T21:45:41.119Z
Learning: Applies to **/*api-client.test.ts : Error tests in API client test files should use the preferred pattern: call '.pipe(E.exit)' on the service call, check 'Exit.isFailure(result)', extract the error with 'getExitError', and assert error details.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Test tagged errors with proper Effect patterns (using `E.exit` and `getExitError`).
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:45:41.119Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/api-client-testing.mdc:0-0
Timestamp: 2025-08-10T21:45:41.119Z
Learning: Applies to **/*api-client.test.ts : When asserting errors in API client test files, check for failure with 'Exit.isFailure(result)', extract error details with 'getExitError', and assert error type and message.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Use `Exit.isFailure(result)` to check for failures and `getExitError(result)` to extract error details in tests.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:45:41.119Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/api-client-testing.mdc:0-0
Timestamp: 2025-08-10T21:45:41.119Z
Learning: Applies to **/*api-client.test.ts : API client test files must import { describe, expect, it } from 'effect/vitest', { FetchHttpClientTest, getExitError } from 't6c/lib-api', { Effect as E, Exit } from 'effect', and { MockConfigLayer } from '../config.js'.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:45:41.119Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/api-client-testing.mdc:0-0
Timestamp: 2025-08-10T21:45:41.119Z
Learning: Applies to **/*api-client.test.ts : API client test files should follow the standard test structure: use 'it.effect' with an Effect generator, provide layers in the correct order, and assert results using 'expect'.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:47:41.747Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-usecase-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:41.747Z
Learning: Applies to **/*-usecase.ts : Always use the required observability pattern in usecase files: .pipe(E.tapError(E.logError), E.orDie or let domain errors bubble up, E.withSpan with only primitive attributes).
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-filters.ts
📚 Learning: 2025-08-10T21:45:57.853Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/code-style.mdc:0-0
Timestamp: 2025-08-10T21:45:57.853Z
Learning: Applies to src/**/*.ts : Use Effect's logging utilities instead of `console.log`; include relevant context in log messages and use appropriate log levels.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:45:57.853Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/code-style.mdc:0-0
Timestamp: 2025-08-10T21:45:57.853Z
Learning: Applies to src/**/*.ts : Use pipe syntax for effect composition; chain operations logically and keep pipes readable with proper formatting.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Use `it.scoped` for tests involving resources that need proper cleanup in Effect-TS test files.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:46:38.101Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-service-architecture.mdc:0-0
Timestamp: 2025-08-10T21:46:38.101Z
Learning: Applies to **/usecase.test.{ts,js} : Usecase testing should use Effect-TS testing utilities, provide mock store implementations, and verify business logic and logging.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.test.ts
📚 Learning: 2025-08-10T21:47:41.747Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-usecase-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:41.747Z
Learning: Applies to **/*-usecase.ts : UPDATE/DELETE usecases must use E.orDie for unexpected errors and use only simple error logging with E.tapError(E.logError).
Applied to files:
src/domain/media/subtitle-formats/subtitle-filters.ts
📚 Learning: 2025-08-10T21:45:57.853Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/code-style.mdc:0-0
Timestamp: 2025-08-10T21:45:57.853Z
Learning: Applies to src/**/*.ts : Use `Data.Tagged` for domain errors, create specific error types for different failure cases, and handle errors explicitly with `E.catchTags`.
Applied to files:
src/domain/media/subtitle-formats/subtitle-filters.ts
📚 Learning: 2025-08-10T21:47:41.747Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-usecase-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:41.747Z
Learning: Applies to **/*-usecase.ts : Always use E.tapError(E.logError) for error logging in usecase files; never use custom error messages, JSON.stringify, or verbose logging.
Applied to files:
src/domain/media/subtitle-formats/subtitle-filters.ts
🧬 Code Graph Analysis (3)
src/domain/media/subtitle-formats/subtitle-converter.test.ts (4)
src/domain/media/subtitle-formats/subtitle-converter.ts (7)
SubtitleItem(740-740)validateSubtitleData(106-150)processSubtitles(304-343)SubtitleConverterLive(659-737)runSubtitleProcessingStream(385-393)runSubtitleConversionStream(480-487)addSpeakerInfo(187-198)src/domain/media/subtitle-formats/subtitle-formats.schema.ts (2)
SubtitleItem(7-16)SubtitleItem(64-64)src/domain/media/subtitle-formats/subtitle-filters.ts (6)
applyFiltersToArray(225-237)replaceText(19-37)addTimingOffset(45-51)filterBySpeaker(59-62)addPrefix(84-89)streamSubtitles(246-259)src/domain/media/subtitle-formats/subtitle-formats.errors.ts (2)
UnsupportedFormatError(43-48)InvalidTimingError(50-52)
src/domain/media/subtitle-formats/subtitle-filters.ts (4)
src/domain/media/subtitle-formats/subtitle-formats.schema.ts (2)
SubtitleItem(7-16)SubtitleItem(64-64)src/domain/media/subtitle-formats/subtitle-pipeline-simple.ts (4)
filter(169-172)stream(161-164)applyFilters(79-93)applySingleFilter(66-74)src/steps/media/persist-subtitles.step.ts (1)
subtitlesJson(3-14)src/usecases/media/transcribe-workflow.usecase.ts (2)
transcribeMediaStep(51-52)persistSubtitlesStep(55-56)
src/domain/media/subtitle-formats/subtitle-pipeline-simple.ts (3)
src/domain/media/subtitle-formats/subtitle-converter.ts (3)
SubtitleItem(740-740)formatTimeSrt(495-501)formatTimeVtt(509-515)src/domain/media/subtitle-formats/subtitle-formats.schema.ts (2)
SubtitleItem(7-16)SubtitleItem(64-64)src/domain/media/subtitle-formats/subtitle-filters.ts (1)
applyFilters(305-320)
🔇 Additional comments (2)
src/domain/media/subtitle-formats/subtitle-converter.test.ts (2)
1-21: test: imports and relative paths — LGTM
- Uses @effect/vitest correctly.
- Internal imports are all relative.
- External packages are used via package imports only.
1491-1513: test: add coverage for unsupported format via stream conversionWe assert timing errors in streaming, but not UnsupportedFormatError for
runSubtitleConversionStream. Add a test to cover this error path to fully exercise typed errors.You can append this test near the other stream conversion tests:
it.effect('should surface UnsupportedFormatError in stream conversion', () => E.gen(function* () { const result = yield* runSubtitleConversionStream( sampleSubtitles, 'unsupported' as never, ) const hasError = typeof result === 'object' && result !== null && 'error' in result expect(hasError).toBe(true) if (hasError && 'error' in result) { expect(result.error).toBeInstanceOf(UnsupportedFormatError) } }), )If helpful, I can push a patch with this addition.
⛔ Skipped due to learnings
Learnt from: CR PR: BleedingDev/coursition-effect-experiment#0 File: .rules/effect-testing-patterns.mdc:0-0 Timestamp: 2025-08-10T21:47:21.625Z Learning: Applies to **/*.test.ts : Test tagged errors with proper Effect patterns (using `E.exit` and `getExitError`).
| it.effect('should reject invalid subtitle data', () => | ||
| E.gen(function* () { | ||
| const result = yield* validateSubtitleData( | ||
| invalidSubtitles as SubtitleItem[], | ||
| ) | ||
| expect('cause' in result).toBe(true) | ||
| }).pipe(E.catchAll(E.succeed)), | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
test: prefer Exit pattern over catchAll for error assertions (invalid data)
Adopt the Effect Exit pattern for cleaner, side‑effect‑free error assertions (per testing guidelines). This removes the need to pipe the whole generator through catchAll.
Apply this diff:
- import { Effect as E } from 'effect'
+ import { Effect as E, Cause, Exit } from 'effect'
@@
- it.effect('should reject invalid subtitle data', () =>
- E.gen(function* () {
- const result = yield* validateSubtitleData(
- invalidSubtitles as SubtitleItem[],
- )
- expect('cause' in result).toBe(true)
- }).pipe(E.catchAll(E.succeed)),
- )
+ it.effect('should reject invalid subtitle data', () =>
+ E.gen(function* () {
+ const exit = yield* validateSubtitleData(
+ invalidSubtitles as SubtitleItem[],
+ ).pipe(E.exit)
+ expect(Exit.isFailure(exit)).toBe(true)
+ }),
+ )Also applies to: 2-2
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-converter.test.ts around lines 67
to 75, the test currently pipes the generator through E.catchAll to swallow
failures; replace this with the Effect Exit pattern: run or exit the Effect to
obtain its Exit value (without catchAll) and assert on that Exit being a failure
(e.g., check exit indicates a failure or that 'cause' exists). Remove the
.pipe(E.catchAll(E.succeed)) usage and update the expectation to inspect the
Exit directly.
| it.effect('should reject empty subtitle array', () => | ||
| E.gen(function* () { | ||
| const result = yield* validateSubtitleData([]) | ||
| expect('cause' in result).toBe(true) | ||
| if ('cause' in result && result.cause instanceof Error) { | ||
| expect(result.cause.message).toBe( | ||
| 'Subtitle data must be a non-empty array', | ||
| ) | ||
| } | ||
| }).pipe(E.catchAll(E.succeed)), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
test: assert error message via Exit + Cause instead of catchAll (empty array)
Keep the message assertion, but derive it from the Exit’s Cause to follow the preferred pattern.
- it.effect('should reject empty subtitle array', () =>
- E.gen(function* () {
- const result = yield* validateSubtitleData([])
- expect('cause' in result).toBe(true)
- if ('cause' in result && result.cause instanceof Error) {
- expect(result.cause.message).toBe(
- 'Subtitle data must be a non-empty array',
- )
- }
- }).pipe(E.catchAll(E.succeed)),
- )
+ it.effect('should reject empty subtitle array', () =>
+ E.gen(function* () {
+ const exit = yield* validateSubtitleData([]).pipe(E.exit)
+ expect(Exit.isFailure(exit)).toBe(true)
+ if (Exit.isFailure(exit)) {
+ const errorOpt = Cause.failureOption(exit.cause)
+ if (Option.isSome(errorOpt) && errorOpt.value instanceof Error) {
+ expect(errorOpt.value.message).toBe(
+ 'Subtitle data must be a non-empty array',
+ )
+ }
+ }
+ }),
+ )📝 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.
| it.effect('should reject empty subtitle array', () => | |
| E.gen(function* () { | |
| const result = yield* validateSubtitleData([]) | |
| expect('cause' in result).toBe(true) | |
| if ('cause' in result && result.cause instanceof Error) { | |
| expect(result.cause.message).toBe( | |
| 'Subtitle data must be a non-empty array', | |
| ) | |
| } | |
| }).pipe(E.catchAll(E.succeed)), | |
| ) | |
| it.effect('should reject empty subtitle array', () => | |
| E.gen(function* () { | |
| const exit = yield* validateSubtitleData([]).pipe(E.exit) | |
| expect(Exit.isFailure(exit)).toBe(true) | |
| if (Exit.isFailure(exit)) { | |
| const errorOpt = Cause.failureOption(exit.cause) | |
| if (Option.isSome(errorOpt) && errorOpt.value instanceof Error) { | |
| expect(errorOpt.value.message).toBe( | |
| 'Subtitle data must be a non-empty array', | |
| ) | |
| } | |
| } | |
| }), | |
| ) |
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-converter.test.ts around lines
76-86, the test currently uses catchAll to unwrap the validation failure; change
it to obtain the Effect's Exit and assert the error via its Cause instead.
Replace the .pipe(E.catchAll(E.succeed)) approach with running the effect to get
its Exit (using the project's run/exit helper for Effects), assert the Exit
represents failure, extract the Cause from the Exit and assert its message
equals "Subtitle data must be a non-empty array", keeping the rest of the test
structure unchanged.
| it.effect('should reject null subtitle data', () => | ||
| E.gen(function* () { | ||
| const result = yield* validateSubtitleData(null as never) | ||
| expect('cause' in result).toBe(true) | ||
| }).pipe(E.catchAll(E.succeed)), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
test: use Exit pattern for null input validation
Consistent with the above, assert failure via Exit.
- it.effect('should reject null subtitle data', () =>
- E.gen(function* () {
- const result = yield* validateSubtitleData(null as never)
- expect('cause' in result).toBe(true)
- }).pipe(E.catchAll(E.succeed)),
- )
+ it.effect('should reject null subtitle data', () =>
+ E.gen(function* () {
+ const exit = yield* validateSubtitleData(null as never).pipe(E.exit)
+ expect(Exit.isFailure(exit)).toBe(true)
+ }),
+ )📝 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.
| it.effect('should reject null subtitle data', () => | |
| E.gen(function* () { | |
| const result = yield* validateSubtitleData(null as never) | |
| expect('cause' in result).toBe(true) | |
| }).pipe(E.catchAll(E.succeed)), | |
| ) | |
| it.effect('should reject null subtitle data', () => | |
| E.gen(function* () { | |
| const exit = yield* validateSubtitleData(null as never).pipe(E.exit) | |
| expect(Exit.isFailure(exit)).toBe(true) | |
| }), | |
| ) |
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-converter.test.ts around lines 88
to 93, the test currently asserts a failure by checking for a 'cause' property
on the result; update it to follow the Exit pattern used elsewhere by asserting
that the effect produced an Exit failure. Replace the ad-hoc 'cause' check with
the proper Exit-based assertion (e.g., use the library's Exit/Effect match or
isFailure helper to assert the result is an Exit failure) so the test uniformly
verifies null input validation via Exit.
| it.effect('should process subtitles and print valid SRT file', () => | ||
| E.gen(function* () { | ||
| const complexSubtitles: SubtitleItem[] = [ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
test: rename “print” to “produce” in test descriptions
These tests don’t print; they produce and assert content. Rename for accuracy and to avoid confusion.
- it.effect('should process subtitles and print valid SRT file', () =>
+ it.effect('should process subtitles and produce valid SRT content', () =>
@@
- it.effect('should process subtitles and print valid JSON format', () =>
+ it.effect('should process subtitles and produce valid JSON content', () =>
@@
- it.effect('should process subtitles and print valid VTT format', () =>
+ it.effect('should process subtitles and produce valid VTT content', () =>
@@
- it.effect(
- 'should process subtitles and print valid plain text format',
+ it.effect(
+ 'should process subtitles and produce valid plain text content',
() =>
@@
- it.effect('should process subtitles and print all formats for comparison', () =>
+ it.effect('should process subtitles and produce all formats for comparison', () =>Also applies to: 263-265, 351-353, 420-423, 496-499
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-converter.test.ts around lines
180-182 (and also update occurrences at 263-265, 351-353, 420-423, 496-499),
rename the test description text by replacing the word "print" with "produce"
(e.g., "should print valid SRT file" -> "should produce valid SRT file") so
descriptions accurately reflect that tests produce/assert content; update the
string literals in the it.effect/test declaration lines only, keeping the rest
of each test body unchanged.
| it.effect('should handle errors in pipes', () => | ||
| E.gen(function* () { | ||
| const result = yield* E.succeed(invalidSubtitles).pipe( | ||
| E.flatMap((subtitles) => | ||
| SubtitleConverterLive.convert(subtitles as SubtitleItem[], 'json'), | ||
| ), | ||
| E.catchAll((error) => E.succeed(error)), | ||
| ) | ||
|
|
||
| // The first validation error will be InvalidTimingError for negative start time | ||
| expect(result).toBeInstanceOf(InvalidTimingError) | ||
| }), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
test: avoid catching the entire pipe when asserting errors; use Exit
Catching the whole pipe with catchAll obscures failure signals. Prefer Exit to assert the specific tagged error with no side effects.
- it.effect('should handle errors in pipes', () =>
- E.gen(function* () {
- const result = yield* E.succeed(invalidSubtitles).pipe(
- E.flatMap((subtitles) =>
- SubtitleConverterLive.convert(subtitles as SubtitleItem[], 'json'),
- ),
- E.catchAll((error) => E.succeed(error)),
- )
-
- // The first validation error will be InvalidTimingError for negative start time
- expect(result).toBeInstanceOf(InvalidTimingError)
- }),
- )
+ it.effect('should handle errors in pipes', () =>
+ E.gen(function* () {
+ const exit = yield* E.succeed(invalidSubtitles).pipe(
+ E.flatMap((subtitles) =>
+ SubtitleConverterLive.convert(subtitles as SubtitleItem[], 'json'),
+ ),
+ E.exit,
+ )
+
+ expect(Exit.isFailure(exit)).toBe(true)
+ if (Exit.isFailure(exit)) {
+ const errorOpt = Cause.failureOption(exit.cause)
+ expect(Option.isSome(errorOpt)).toBe(true)
+ if (Option.isSome(errorOpt)) {
+ expect(errorOpt.value).toBeInstanceOf(InvalidTimingError)
+ }
+ }
+ }),
+ )📝 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.
| it.effect('should handle errors in pipes', () => | |
| E.gen(function* () { | |
| const result = yield* E.succeed(invalidSubtitles).pipe( | |
| E.flatMap((subtitles) => | |
| SubtitleConverterLive.convert(subtitles as SubtitleItem[], 'json'), | |
| ), | |
| E.catchAll((error) => E.succeed(error)), | |
| ) | |
| // The first validation error will be InvalidTimingError for negative start time | |
| expect(result).toBeInstanceOf(InvalidTimingError) | |
| }), | |
| ) | |
| it.effect('should handle errors in pipes', () => | |
| E.gen(function* () { | |
| const exit = yield* E.succeed(invalidSubtitles).pipe( | |
| E.flatMap((subtitles) => | |
| SubtitleConverterLive.convert(subtitles as SubtitleItem[], 'json'), | |
| ), | |
| E.exit, | |
| ) | |
| expect(Exit.isFailure(exit)).toBe(true) | |
| if (Exit.isFailure(exit)) { | |
| const errorOpt = Cause.failureOption(exit.cause) | |
| expect(Option.isSome(errorOpt)).toBe(true) | |
| if (Option.isSome(errorOpt)) { | |
| expect(errorOpt.value).toBeInstanceOf(InvalidTimingError) | |
| } | |
| } | |
| }), | |
| ) |
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-converter.test.ts around lines
1352 to 1364, the test is catching the entire pipe with E.catchAll which hides
the effect failure; instead run the effect to an Exit (use E.exit or equivalent
to get an Exit<E, A>) and assert the Exit is a failure containing the specific
InvalidTimingError (no side-effectful catch). Replace the current
E.catchAll(...) usage with an Exit-producing call, inspect Exit.failure to
verify the error is the expected InvalidTimingError (or use
Exit.match/Exit.failureOption to assert the tagged error) and remove the
catchAll-based succeed branch so the test fails correctly on unexpected
outcomes.
| describe('Middleware filter debug', () => { | ||
| it('should print subtitles before and after each filter', () => { | ||
| const originalSubtitles: SubtitleItem[] = [ | ||
| { start: 0, end: 2000, text: 'First line', speaker: 1 }, | ||
| { start: 2000, end: 4000, text: 'Second line', speaker: 2 }, | ||
| ] | ||
|
|
||
| // Apply addTimingOffset | ||
| const offsetSubtitles = originalSubtitles.map(addTimingOffset(1000)) | ||
|
|
||
| // Apply replaceText | ||
| const replacedSubtitles = offsetSubtitles.map(replaceText('Replaced!')) | ||
|
|
||
| // Apply addPrefix | ||
| const prefixedSubtitles = replacedSubtitles.map(addPrefix('[PREFIX]')) | ||
|
|
||
| // Final assertion (just to keep the test green) | ||
| expect(prefixedSubtitles[0]?.text).toBe('[PREFIX] Replaced!') | ||
| expect(prefixedSubtitles[1]?.text).toBe('[PREFIX] Replaced!') | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
test: avoid “debug” wording; make intent explicit
The block no longer logs. Rename to reflect behaviour.
- describe('Middleware filter debug', () => {
- it('should print subtitles before and after each filter', () => {
+ describe('Composed middleware filters (no logs)', () => {
+ it('should transform subtitles with each filter applied', () => {📝 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.
| describe('Middleware filter debug', () => { | |
| it('should print subtitles before and after each filter', () => { | |
| const originalSubtitles: SubtitleItem[] = [ | |
| { start: 0, end: 2000, text: 'First line', speaker: 1 }, | |
| { start: 2000, end: 4000, text: 'Second line', speaker: 2 }, | |
| ] | |
| // Apply addTimingOffset | |
| const offsetSubtitles = originalSubtitles.map(addTimingOffset(1000)) | |
| // Apply replaceText | |
| const replacedSubtitles = offsetSubtitles.map(replaceText('Replaced!')) | |
| // Apply addPrefix | |
| const prefixedSubtitles = replacedSubtitles.map(addPrefix('[PREFIX]')) | |
| // Final assertion (just to keep the test green) | |
| expect(prefixedSubtitles[0]?.text).toBe('[PREFIX] Replaced!') | |
| expect(prefixedSubtitles[1]?.text).toBe('[PREFIX] Replaced!') | |
| }) | |
| }) | |
| describe('Composed middleware filters (no logs)', () => { | |
| it('should transform subtitles with each filter applied', () => { | |
| const originalSubtitles: SubtitleItem[] = [ | |
| { start: 0, end: 2000, text: 'First line', speaker: 1 }, | |
| { start: 2000, end: 4000, text: 'Second line', speaker: 2 }, | |
| ] | |
| // Apply addTimingOffset | |
| const offsetSubtitles = originalSubtitles.map(addTimingOffset(1000)) | |
| // Apply replaceText | |
| const replacedSubtitles = offsetSubtitles.map(replaceText('Replaced!')) | |
| // Apply addPrefix | |
| const prefixedSubtitles = replacedSubtitles.map(addPrefix('[PREFIX]')) | |
| // Final assertion (just to keep the test green) | |
| expect(prefixedSubtitles[0]?.text).toBe('[PREFIX] Replaced!') | |
| expect(prefixedSubtitles[1]?.text).toBe('[PREFIX] Replaced!') | |
| }) | |
| }) |
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-converter.test.ts around lines
1447 to 1467, the test and describe block use the word "debug" despite not
performing logging; rename them to explicitly state the intent (e.g., describe:
"Middleware filter transformation" or "Middleware filter pipeline", it: "should
apply timing offset, replaceText and addPrefix in sequence producing expected
texts") so the names reflect behavior rather than debugging; update only the
describe and it strings to the new explicit wording, leaving the test body and
assertions unchanged.
| // Type guard to check if result has error property | ||
| const hasError = | ||
| typeof result === 'object' && result !== null && 'error' in result | ||
| expect(hasError).toBe(false) | ||
|
|
||
| if (!hasError && Array.isArray(result)) { | ||
| expect(result).toHaveLength(3) | ||
| expect(result[0]?.start).toBe(1000) | ||
| expect(result[0]?.end).toBe(6000) | ||
| expect(result[2]?.text).toBe('[Speaker 1]: Subtitle processing') | ||
| } | ||
| }), |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
test: factor repeated error type‑guard into helper
The “hasError” type guard is duplicated. Introduce a small helper to improve readability and reduce repetition.
Add once near the top of the Streaming Processing describe:
const isErrorResult = (x: unknown): x is { error: unknown } =>
typeof x === 'object' && x !== null && 'error' in xThen replace occurrences of the inline type guards with isErrorResult(result).
Also applies to: 1502-1511, 1523-1561, 1592-1600
🤖 Prompt for AI Agents
src/domain/media/subtitle-formats/subtitle-converter.test.ts lines 1477-1488
(and also apply to ranges 1502-1511, 1523-1561, 1592-1600): factor the repeated
inline type-guard into a single helper by adding a helper named isErrorResult
near the top of the "Streaming Processing" describe block that narrows unknown
to { error: unknown }, then replace every inline guard expression (typeof x ===
'object' && x !== null && 'error' in x) with isErrorResult(result) in the listed
ranges so tests use the helper consistently.
| it.effect('should save subtitle content to file using Bun FS', () => | ||
| E.gen(function* () { | ||
| const complexSubtitles: SubtitleItem[] = [ | ||
| { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
test: clarify FS usage in title (no Bun FS calls here)
Title mentions Bun FS, but the test uses an in‑memory Effect‑style mock. Rename to avoid misleading future readers.
- it.effect('should save subtitle content to file using Bun FS', () =>
+ it.effect('should save subtitle content to file using an in-memory FS (Effect Platform mock)', () =>📝 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.
| it.effect('should save subtitle content to file using Bun FS', () => | |
| E.gen(function* () { | |
| const complexSubtitles: SubtitleItem[] = [ | |
| { | |
| it.effect('should save subtitle content to file using an in-memory FS (Effect Platform mock)', () => | |
| E.gen(function* () { | |
| const complexSubtitles: SubtitleItem[] = [ | |
| { |
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-converter.test.ts around lines
1603 to 1606, the test title incorrectly mentions "Bun FS" while the test uses
an in-memory Effect-style mock; update the test description to accurately
reflect the implementation (e.g., "should save subtitle content to file using
in-memory FS mock" or "should save subtitle content to file using mocked FS") so
readers aren’t misled, leaving the test body unchanged.
There was a problem hiding this comment.
Actionable comments posted: 27
♻️ Duplicate comments (10)
src/domain/media/subtitle-formats/README.md (2)
111-119: docs(tooling): switch README commands to pnpm to match workspace package managerThe repository uses pnpm. Updating the examples avoids confusion for contributors.
Apply this diff:
#### **Option 2: NPM Scripts** ```bash -# From project root -npm run test:subtitles # Run all subtitle tests -npm run test:subtitles:watch # Watch mode - -# From subtitle directory -npm test -- test-enhanced-endpoints.test.ts +# From project root +pnpm run test:subtitles # Run all subtitle tests +pnpm run test:subtitles:watch # Watch mode + +# From subtitle directory (root script path still applies) +pnpm test -- src/domain/media/subtitle-formats/test-enhanced-endpoints.test.ts--- `1-18`: **docs(subtitles): add “Streaming APIs” section for runSubtitleProcessingStream/runSubtitleConversionStream** Per PR objectives, the new streaming functions are public APIs but aren’t documented here. Add a concise “Streaming APIs” section (with short examples) right after “Usage Examples”, covering: - runSubtitleProcessingStream(items, filters?, config?) — consumes (async) iterables and yields processed items. - runSubtitleConversionStream(items, filters?, config?) — yields incremental SRT/VTT/Text/JSON fragments as items arrive. I can draft the section with runnable snippets aligned to `subtitle-converter.ts` if helpful. Also applies to: 190-237 </blockquote></details> <details> <summary>src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts (3)</summary><blockquote> `1-1`: **Use `@effect/vitest` for Effect-based tests** According to the coding guidelines, tests involving Effect-based code should import from `@effect/vitest`, not regular `vitest`. ```diff -import { describe, expect, it } from 'vitest' +import { describe, expect, it } from '@effect/vitest'
132-132: Fix expected SRT text assertionThe
formatToSrtfunction preserves the original text including punctuation. Your test data contains 'Hello, world.' but the expectation is 'Hello world'.- expect(strings[2]).toBe('Hello world') + expect(strings[2]).toBe('Hello, world.')
344-356: Remove time-based performance assertionsUsing
Date.now()in tests is brittle and environment-dependent. Performance assertions should be in dedicated benchmark tests, not unit tests.- const startTime = Date.now() const result = createArrayPipeline(largeDataset) .filter( applyFilters(validateSubtitle, toUpperCase, addPrefix('[PROCESSED]')), ) .collector(createCollector()) .formatter(formatToSrt) .execute() - const endTime = Date.now() expect(result).toBeInstanceOf(Array) expect((result as string[]).length).toBeGreaterThan(0) - expect(endTime - startTime).toBeLessThan(1000) // Should complete within 1 secondsrc/domain/media/subtitle-formats/subtitle-formats.schema.ts (5)
4-16: style(docs): reduce obvious inline comments; prefer concise JSDoc on public APIsField-level comments restating names/types add noise and drift risk. Keep top-level JSDoc where it adds context; remove obvious per-field comments.
Example clean-up:
-/** - * Represents a single subtitle item with timing and text content - */ export const SubtitleItem = …Also applies to: 18-43, 51-75, 77-94
44-49: fix(schema): use type-level of SubtitleFormat in TaggedClassUsing the runtime value in the generic will not type-check; use S.Type.
export class SubtitleConversionResult extends Data.TaggedClass( 'SubtitleConversionResult', )<{ - format: SubtitleFormat + format: S.Type<typeof SubtitleFormat> content: string }> {}
59-61: fix(schema): align MultipleFormatResult to S alias-export const MultipleFormatResult = Schema.Struct({ - results: Schema.Array(SubtitleConversionResultSchema), -}) +export const MultipleFormatResult = S.Struct({ + results: S.Array(SubtitleConversionResultSchema), +})
1-2: style(schema): import Schema as S and standardise usageAdopt the repository convention to import Schema as S and update all builders accordingly.
-import { Schema } from 'effect' -import { Data } from 'effect' +import { Schema as S, Data } from 'effect'
95-104: refactor(types): replace allSchema.Schema.TypewithS.TypeTo align with our code-style rules and fix the compile-time errors, every occurrence of
Schema.Schema.Type<typeof X>should be changed to
S.Type<typeof X>and each file importing
Schemamust instead alias it asS(for example:import * as S from '@effect/schema').Affected locations include, but may not be limited to:
- src/domain/media/subtitle-formats/subtitle-formats.schema.ts (lines 96–104)
- src/domain/media/subtitle-formats/endpoints.ts (lines 109–110)
- src/domain/media/subtitle-formats/subtitle-converter.ts (around line 719)
- src/usecases/media/transcribe-workflow.usecase.ts (line 12)
- src/steps/media/download-link.step.ts (line 4)
- src/steps/media/transcribe-media.step.ts (line 4)
- src/handlers/media/transcribe-workflow.handler.ts (line 7)
- src/stores/workflow/workflow-store.ts (lines 10 & 18)
- src/stores/media/media.store.ts (line 24)
Example diff in
subtitle-formats.schema.ts:- export type SubtitleItem = Schema.Schema.Type<typeof SubtitleItem> + export type SubtitleItem = S.Type<typeof SubtitleItem>Please apply this refactoring across the codebase to restore correct typing and comply with our “Schema as S” import convention.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
package.json(1 hunks)src/domain/media/subtitle-formats/README.md(1 hunks)src/domain/media/subtitle-formats/endpoints.test.ts(1 hunks)src/domain/media/subtitle-formats/endpoints.ts(1 hunks)src/domain/media/subtitle-formats/run-tests.sh(1 hunks)src/domain/media/subtitle-formats/subtitle-converter-clean.test.ts(1 hunks)src/domain/media/subtitle-formats/subtitle-formats.schema.ts(1 hunks)src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts(1 hunks)src/domain/media/subtitle-formats/subtitle-processor-enhanced.handler.ts(1 hunks)src/domain/media/subtitle-formats/subtitle-processor-enhanced.test.ts(1 hunks)src/domain/media/subtitle-formats/test-enhanced-endpoints.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.ts
📄 CodeRabbit Inference Engine (.rules/code-style.mdc)
src/**/*.ts: Always use relative paths for internal imports; never usesrc/aliases for internal imports in TypeScript files.
Use package imports only for external dependencies in TypeScript files.
Avoid obvious comments; never add comments that just repeat what the code is doing. Only add comments for complex business logic that needs explanation.
Use JSDoc for public APIs, not inline comments for obvious operations.
Add comments only for complex business logic, non-obvious workarounds, edge cases, or important context that affects future changes.
UseEffect.Serviceclass extension pattern for services, include dependencies in service definition, and providemakeTestServicefor testing.
UseE.gen(function* () {})for complex flows; yield effects directly without helper functions; useyield*for effects that return values.
UseData.Taggedfor domain errors, create specific error types for different failure cases, and handle errors explicitly withE.catchTags.
Use Effect's logging utilities instead ofconsole.log; include relevant context in log messages and use appropriate log levels.
Add spans for observability usingE.withSpan, include relevant attributes, and use descriptive span names.
Use PascalCase for service classes (e.g.,BlogStore,SearchStore).
Use camelCase for functions and variables (e.g.,getBlogById,blogPost).
Use UPPERCASE for constants (e.g.,TABLE_NAME,DEFAULT_LIMIT).
Use descriptive names for functions and methods that indicate purpose.
Prefix usecases with action (e.g.,createBlogPostUsecase,getBlogByIdUsecase).
Prefix handlers with HTTP method context (e.g.,_handler,handler).
Use Effect Schema for all data validation; define schemas with explicit types and use branded types for domain identifiers.
Use branded types for domain identifiers and create factory functions with validation.
Access configuration through the Effect system, not directly viaprocess.env.
Use pipe syntax for effect composition;...
Files:
src/domain/media/subtitle-formats/subtitle-processor-enhanced.test.tssrc/domain/media/subtitle-formats/subtitle-converter-clean.test.tssrc/domain/media/subtitle-formats/test-enhanced-endpoints.test.tssrc/domain/media/subtitle-formats/endpoints.test.tssrc/domain/media/subtitle-formats/subtitle-formats.schema.tssrc/domain/media/subtitle-formats/endpoints.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.test.tssrc/domain/media/subtitle-formats/subtitle-processor-enhanced.handler.ts
src/**/*
📄 CodeRabbit Inference Engine (.rules/code-style.mdc)
Use kebab-case for file names (e.g.,
blog.store.ts,get-blog-by-id.ts).
Files:
src/domain/media/subtitle-formats/subtitle-processor-enhanced.test.tssrc/domain/media/subtitle-formats/subtitle-converter-clean.test.tssrc/domain/media/subtitle-formats/test-enhanced-endpoints.test.tssrc/domain/media/subtitle-formats/endpoints.test.tssrc/domain/media/subtitle-formats/run-tests.shsrc/domain/media/subtitle-formats/subtitle-formats.schema.tssrc/domain/media/subtitle-formats/endpoints.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.test.tssrc/domain/media/subtitle-formats/subtitle-processor-enhanced.handler.tssrc/domain/media/subtitle-formats/README.md
src/**/*.test.ts
📄 CodeRabbit Inference Engine (.rules/code-style.mdc)
Always use
@effect/vitestfor Effect-based tests; use appropriate test variants (it.effect,it.scoped,it.live); mock services usingmakeTestService.
Files:
src/domain/media/subtitle-formats/subtitle-processor-enhanced.test.tssrc/domain/media/subtitle-formats/subtitle-converter-clean.test.tssrc/domain/media/subtitle-formats/test-enhanced-endpoints.test.tssrc/domain/media/subtitle-formats/endpoints.test.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.rules/effect-service-architecture.mdc)
Always use relative paths for internal imports; never use
src/aliases for internal imports. Use package imports only for external dependencies.
Files:
src/domain/media/subtitle-formats/subtitle-processor-enhanced.test.tssrc/domain/media/subtitle-formats/subtitle-converter-clean.test.tssrc/domain/media/subtitle-formats/test-enhanced-endpoints.test.tssrc/domain/media/subtitle-formats/endpoints.test.tssrc/domain/media/subtitle-formats/subtitle-formats.schema.tssrc/domain/media/subtitle-formats/endpoints.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.test.tssrc/domain/media/subtitle-formats/subtitle-processor-enhanced.handler.ts
**/*.test.ts
📄 CodeRabbit Inference Engine (.rules/effect-testing-patterns.mdc)
**/*.test.ts: Always use@effect/vitestfor tests involving Effect-based code; import test functions (describe,expect,it) from@effect/vitest, not regularvitest.
Always use relative paths for internal imports in test files; never usesrc/aliases.
Use package imports for external dependencies in test files.
Test tagged errors with proper Effect patterns (usingE.exitandgetExitError).
Useit.effectfor tests that need TestContext (TestClock, etc.) in Effect-TS test files.
Useit.scopedfor tests involving resources that need proper cleanup in Effect-TS test files.
Useit.livefor tests that need real system time and live services in Effect-TS test files.
UseFetchHttpClientTestfrom@t6c/lib-apifor mocking HTTP responses in API client tests.
Use mock data from domain packages when available in tests.
Create reusable mock objects for complex data structures in tests.
Use proper branded type constructors for mock data in tests.
Declare variables outside generator functions when used inFetchHttpClientTestto prevent scoping issues in tests.
Use.pipe(E.exit)directly on service calls for cleaner error testing in tests; avoid nestedE.exit(E.gen(...))patterns.
UseExit.isFailure(result)to check for failures andgetExitError(result)to extract error details in tests.
Test all public methods of services, including both success and error scenarios, default parameters, edge cases, and validation.
Use appropriate HTTP status codes (200, 201, 404, 500) and match response structure to actual API responses in mock responses for tests.
Use descriptive test names that explain the scenario, following the pattern: 'should [action] [condition]'.
Group related tests in describe blocks by method.
Reuse mock data across tests when possible.
Create typed mock objects with proper constructors for tests.
Always provide required layers in correct order in tests (e.g.,DefaultWithoutDependenciesfor services,MockConfigLayerfor configurat...
Files:
src/domain/media/subtitle-formats/subtitle-processor-enhanced.test.tssrc/domain/media/subtitle-formats/subtitle-converter-clean.test.tssrc/domain/media/subtitle-formats/test-enhanced-endpoints.test.tssrc/domain/media/subtitle-formats/endpoints.test.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
🧠 Learnings (22)
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Use proper branded type constructors for mock data in tests.
Applied to files:
src/domain/media/subtitle-formats/subtitle-processor-enhanced.test.tssrc/domain/media/subtitle-formats/subtitle-formats.schema.ts
📚 Learning: 2025-08-10T21:45:41.119Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/api-client-testing.mdc:0-0
Timestamp: 2025-08-10T21:45:41.119Z
Learning: Applies to **/*api-client.test.ts : API client test files must import { describe, expect, it } from 'effect/vitest', { FetchHttpClientTest, getExitError } from 't6c/lib-api', { Effect as E, Exit } from 'effect', and { MockConfigLayer } from '../config.js'.
Applied to files:
src/domain/media/subtitle-formats/subtitle-processor-enhanced.test.tssrc/domain/media/subtitle-formats/test-enhanced-endpoints.test.tssrc/domain/media/subtitle-formats/endpoints.test.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Test all public methods of services, including both success and error scenarios, default parameters, edge cases, and validation.
Applied to files:
src/domain/media/subtitle-formats/test-enhanced-endpoints.test.ts
📚 Learning: 2025-08-10T21:45:41.119Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/api-client-testing.mdc:0-0
Timestamp: 2025-08-10T21:45:41.119Z
Learning: Applies to **/*api-client.test.ts : Test all public service methods in API client test files, including both success and error scenarios, default parameters, edge cases, and validation.
Applied to files:
src/domain/media/subtitle-formats/test-enhanced-endpoints.test.tssrc/domain/media/subtitle-formats/endpoints.test.ts
📚 Learning: 2025-08-10T21:45:41.119Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/api-client-testing.mdc:0-0
Timestamp: 2025-08-10T21:45:41.119Z
Learning: Applies to **/*api-client.test.ts : API client test files should follow the standard test structure: use 'it.effect' with an Effect generator, provide layers in the correct order, and assert results using 'expect'.
Applied to files:
src/domain/media/subtitle-formats/endpoints.test.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
📚 Learning: 2025-08-10T21:48:12.331Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/schema-validation.mdc:0-0
Timestamp: 2025-08-10T21:48:12.331Z
Learning: Applies to **/schema/**/*.ts|**/schemas/**/*.ts|**/*-schema.ts|**/*-schemas.ts : Test schema validation with valid and invalid inputs using effect/vitest and Effect-based test patterns
Applied to files:
src/domain/media/subtitle-formats/endpoints.test.tssrc/domain/media/subtitle-formats/subtitle-formats.schema.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
📚 Learning: 2025-08-10T21:45:57.853Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/code-style.mdc:0-0
Timestamp: 2025-08-10T21:45:57.853Z
Learning: Applies to src/**/*.test.ts : Always use `effect/vitest` for Effect-based tests; use appropriate test variants (`it.effect`, `it.scoped`, `it.live`); mock services using `makeTestService`.
Applied to files:
src/domain/media/subtitle-formats/endpoints.test.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Always use `effect/vitest` for tests involving Effect-based code; import test functions (`describe`, `expect`, `it`) from `effect/vitest`, not regular `vitest`.
Applied to files:
src/domain/media/subtitle-formats/endpoints.test.tssrc/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
📚 Learning: 2025-08-10T21:45:57.853Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/code-style.mdc:0-0
Timestamp: 2025-08-10T21:45:57.853Z
Learning: Applies to src/**/*.ts : Use Effect Schema for all data validation; define schemas with explicit types and use branded types for domain identifiers.
Applied to files:
src/domain/media/subtitle-formats/subtitle-formats.schema.ts
📚 Learning: 2025-08-10T21:46:10.977Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/cursor-rule-maintainer.mdc:0-0
Timestamp: 2025-08-10T21:46:10.977Z
Learning: Applies to .cursor/rules/schema-validation.mdc : Add new Effect Schema patterns, validation improvements, branded type patterns, and error handling for schemas to `schema-validation.mdc`
Applied to files:
src/domain/media/subtitle-formats/subtitle-formats.schema.ts
📚 Learning: 2025-08-10T21:48:12.331Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/schema-validation.mdc:0-0
Timestamp: 2025-08-10T21:48:12.331Z
Learning: Applies to **/*.types.ts|**/schema/**/*.ts|**/schemas/**/*.ts|**/*-schema.ts|**/*-schemas.ts|**/branded.ts|**/domain/**/*.ts : Always use `Schema as S` for Effect Schema imports
Applied to files:
src/domain/media/subtitle-formats/subtitle-formats.schema.ts
📚 Learning: 2025-08-10T21:48:12.331Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/schema-validation.mdc:0-0
Timestamp: 2025-08-10T21:48:12.331Z
Learning: Applies to **/*.types.ts|**/schema/**/*.ts|**/schemas/**/*.ts|**/*-schema.ts|**/*-schemas.ts|**/branded.ts|**/domain/**/*.ts : Use `Data.TaggedError` for domain-specific error types and include relevant identifiers and context in error data
Applied to files:
src/domain/media/subtitle-formats/subtitle-formats.schema.ts
📚 Learning: 2025-08-10T21:03:54.779Z
Learnt from: BleedingDev
PR: BleedingDev/coursition-effect-experiment#18
File: src/domain/workflow/workflow.errors.ts:3-6
Timestamp: 2025-08-10T21:03:54.779Z
Learning: In Effect-based applications, Schema.TaggedError is used for serializable errors that can bubble up to clients/external systems, while Data.TaggedError is used for internal errors that stay within the application boundaries. This is an intentional architectural pattern to differentiate between public API errors and internal implementation errors.
Applied to files:
src/domain/media/subtitle-formats/subtitle-formats.schema.ts
📚 Learning: 2025-08-10T21:45:57.853Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/code-style.mdc:0-0
Timestamp: 2025-08-10T21:45:57.853Z
Learning: Applies to src/**/*.ts : Use `Data.Tagged` for domain errors, create specific error types for different failure cases, and handle errors explicitly with `E.catchTags`.
Applied to files:
src/domain/media/subtitle-formats/subtitle-formats.schema.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Never use `throw new Error()` in Effect-TS applications; always create and use `Data.TaggedError` for error handling.
Applied to files:
src/domain/media/subtitle-formats/subtitle-formats.schema.ts
📚 Learning: 2025-08-10T21:48:12.331Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/schema-validation.mdc:0-0
Timestamp: 2025-08-10T21:48:12.331Z
Learning: Applies to **/*.types.ts|**/schema/**/*.ts|**/schemas/**/*.ts|**/*-schema.ts|**/*-schemas.ts|**/branded.ts|**/domain/**/*.ts : Use `S.pipe` with `S.brand` to create type-branded primitives and export both the schema and the type
Applied to files:
src/domain/media/subtitle-formats/subtitle-formats.schema.ts
📚 Learning: 2025-08-10T21:45:28.136Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/api-client-patterns.mdc:0-0
Timestamp: 2025-08-10T21:45:28.136Z
Learning: Applies to **/*.api-client.ts : Import types with the `type` keyword when used only for typing; import schemas without `type` when used for encoding/decoding.
Applied to files:
src/domain/media/subtitle-formats/subtitle-formats.schema.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Use `it.effect` for tests that need TestContext (TestClock, etc.) in Effect-TS test files.
Applied to files:
src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Use `it.live` for tests that need real system time and live services in Effect-TS test files.
Applied to files:
src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Use `it.scoped` for tests involving resources that need proper cleanup in Effect-TS test files.
Applied to files:
src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
📚 Learning: 2025-08-10T21:46:38.101Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-service-architecture.mdc:0-0
Timestamp: 2025-08-10T21:46:38.101Z
Learning: Applies to **/stores/*-store.test.{ts,js} : Store testing should use Effect-TS testing utilities (e.g., `effect/vitest`), provide mock implementations for dependencies, and test store methods in isolation.
Applied to files:
src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Test tagged errors with proper Effect patterns (using `E.exit` and `getExitError`).
Applied to files:
src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts
🧬 Code Graph Analysis (7)
src/domain/media/subtitle-formats/subtitle-processor-enhanced.test.ts (5)
src/domain/media/subtitle-formats/subtitle-converter.ts (1)
SubtitleConverterLive(659-737)src/domain/media/subtitle-formats/subtitle-processor-enhanced.handler.ts (4)
enhancedProcessSubtitlesHandler(30-93)processSubtitlesHandler(98-174)getSupportedFormatsHandler(268-278)healthCheckHandler(281-293)src/domain/media/subtitle-formats/endpoints.ts (2)
ProcessSubtitlesRequest(19-28)ProcessSubtitlesRequest(109-109)src/domain/media/subtitle-formats/subtitle-formats.schema.ts (4)
SubtitleItem(7-16)SubtitleItem(96-96)SubtitleFormat(26-26)SubtitleFormat(98-98)src/domain/media/subtitle-formats/subtitle-formats.errors.ts (4)
SubtitleDataInvalid(4-7)SubtitleFormatUnsupported(9-15)SubtitleConversionFailed(22-27)SubtitleProcessingFailed(29-34)
src/domain/media/subtitle-formats/subtitle-converter-clean.test.ts (4)
src/domain/media/subtitle-formats/subtitle-formats.schema.ts (4)
SubtitleItem(7-16)SubtitleItem(96-96)SubtitleFormat(26-26)SubtitleFormat(98-98)src/domain/media/subtitle-formats/subtitle-converter.ts (6)
SubtitleItem(740-740)validateSubtitleData(106-150)processSubtitles(304-343)SubtitleConverterLive(659-737)runSubtitleProcessingStream(385-393)runSubtitleConversionStream(480-487)src/domain/media/subtitle-formats/subtitle-formats.errors.ts (2)
UnsupportedFormatError(43-48)InvalidTimingError(50-52)src/domain/media/subtitle-formats/subtitle-filters.ts (6)
replaceText(19-37)addTimingOffset(45-51)addPrefix(84-89)filterBySpeaker(59-62)applyFiltersToArray(225-237)streamSubtitles(246-259)
src/domain/media/subtitle-formats/endpoints.test.ts (2)
src/domain/media/subtitle-formats/endpoints.ts (7)
ProcessSubtitlesRequest(19-28)ProcessSubtitlesRequest(109-109)ProcessSubtitlesResponse(33-44)ProcessSubtitlesResponse(110-110)subtitleGroup(98-103)createSubtitleApi(127-128)addSubtitleEndpoints(120-121)src/domain/media/subtitle-formats/subtitle-formats.schema.ts (4)
SubtitleFormat(26-26)SubtitleFormat(98-98)SubtitleItem(7-16)SubtitleItem(96-96)
src/domain/media/subtitle-formats/subtitle-formats.schema.ts (2)
src/domain/media/subtitle-formats/subtitle-converter.ts (1)
SubtitleItem(740-740)src/domain/media/subtitle-formats/subtitle-processor-enhanced.handler.ts (2)
EnhancedProcessSubtitlesRequest(12-17)MultiFormatResponse(19-28)
src/domain/media/subtitle-formats/endpoints.ts (2)
src/domain/media/subtitle-formats/subtitle-formats.schema.ts (10)
SubtitleFormat(26-26)SubtitleFormat(98-98)SubtitleItem(7-16)SubtitleItem(96-96)ConversionOptions(31-42)ConversionOptions(99-99)EnhancedProcessSubtitlesRequest(66-75)EnhancedProcessSubtitlesRequest(103-103)MultiFormatResponse(80-93)MultiFormatResponse(104-104)src/domain/media/subtitle-formats/subtitle-formats.errors.ts (4)
SubtitleDataInvalid(4-7)SubtitleFormatUnsupported(9-15)SubtitleConversionFailed(22-27)SubtitleProcessingFailed(29-34)
src/domain/media/subtitle-formats/subtitle-pipeline-simple.test.ts (3)
src/domain/media/subtitle-formats/subtitle-formats.schema.ts (2)
SubtitleItem(7-16)SubtitleItem(96-96)src/domain/media/subtitle-formats/subtitle-pipeline-simple.ts (13)
createPipeline(305-307)SubtitlePipeline(144-296)createArrayPipeline(312-318)PipelineConfig(43-47)createCollector(121-128)applyFilters(79-93)formatToSrt(327-345)formatToVtt(350-363)formatToPlainText(368-370)formatToJson(375-377)processToSrt(412-421)processToVtt(426-435)processWithConfig(440-454)src/domain/media/subtitle-formats/subtitle-filters.ts (9)
toUpperCase(152-155)applyFilters(305-320)removeEmptySubtitles(182-185)addPrefix(84-89)filterBySpeakers(70-76)addTimingOffset(45-51)transformText(140-145)capitalize(172-175)validateSubtitle(202-216)
src/domain/media/subtitle-formats/subtitle-processor-enhanced.handler.ts (4)
src/domain/media/subtitle-formats/subtitle-formats.schema.ts (10)
EnhancedProcessSubtitlesRequest(66-75)EnhancedProcessSubtitlesRequest(103-103)SubtitleItem(7-16)SubtitleItem(96-96)ConversionOptions(31-42)ConversionOptions(99-99)MultiFormatResponse(80-93)MultiFormatResponse(104-104)SubtitleFormat(26-26)SubtitleFormat(98-98)src/domain/media/subtitle-formats/subtitle-converter.ts (1)
SubtitleConverterLive(659-737)src/domain/media/subtitle-formats/subtitle-formats.errors.ts (4)
SubtitleDataInvalid(4-7)SubtitleFormatUnsupported(9-15)SubtitleConversionFailed(22-27)SubtitleProcessingFailed(29-34)src/domain/media/subtitle-formats/endpoints.ts (4)
ProcessSubtitlesRequest(19-28)ProcessSubtitlesRequest(109-109)ProcessSubtitlesResponse(33-44)ProcessSubtitlesResponse(110-110)
🪛 markdownlint-cli2 (0.17.2)
src/domain/media/subtitle-formats/README.md
7-7: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
8-8: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
13-13: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
14-14: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
19-19: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
20-20: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
25-25: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
26-26: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
33-33: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
34-34: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
36-36: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
39-39: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
40-40: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
42-42: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
45-45: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
46-46: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
70-70: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
71-71: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
100-100: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
101-101: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
104-104: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
106-106: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
111-111: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
112-112: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
121-121: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
122-122: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
159-159: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
192-192: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
193-193: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
206-206: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
207-207: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
223-223: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
224-224: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
241-241: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
242-242: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
254-254: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
255-255: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
261-261: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
263-263: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
268-268: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
270-270: Lists should be surrounded by blank lines
(MD032, blanks-around-lists)
288-288: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
288-288: Files should end with a single newline character
(MD047, single-trailing-newline)
🔇 Additional comments (5)
src/domain/media/subtitle-formats/run-tests.sh (1)
1-48: chore(test): harden and moderniserun-tests.shfor portability, pnpm-first use, and correct exit-code propagationThis update addresses the following:
- Adds
#!/usr/bin/env bashandset -euo pipefailfor strict error handling- Computes
REPO_ROOTandSUB_DIRdynamically so the script can be run from any CWD- Enforces use of
pnpm(matching workspacepackageManager) and fails fast if missing- Captures integration and unit test exit codes into
INTEG_RC/UNIT_RCinstead of assuming success- Prints a summary that reflects actual test outcomes (❌ or ✅ with exit code)
- Only returns success (0) if both test suites pass
Please apply the following diff to
src/domain/media/subtitle-formats/run-tests.sh, then run the updated script and confirm that:
- It still checks the server health on
$PORT(defaults to 3001)- It fails immediately if
pnpmisn’t installed- Integration/unit tests are run via
pnpm run test:subtitles:integrationandpnpm run test:subtitles:unit- The summary shows ❌ with the actual exit code when tests fail
- The final exit code is non-zero if any test suite fails
--- a/src/domain/media/subtitle-formats/run-tests.sh +++ b/src/domain/media/subtitle-formats/run-tests.sh @@ -1,6 +1,10 @@ -#!/bin/bash - -# Enhanced Subtitle Tests Runner -# Run this script from the subtitle-formats directory +#!/usr/bin/env bash +set -euo pipefail + +# Enhanced Subtitle Tests Runner (works from any directory) +REPO_ROOT="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")/../../../.." && pwd)" +SUB_DIR="$REPO_ROOT/src/domain/media/subtitle-formats" +PORT="${PORT:-3001}" +HEALTH_URL="http://localhost:${PORT}/subtitles/health" -echo "🚀 Running Enhanced Subtitle Tests..." -echo "======================================" +echo "🚀 Running Enhanced Subtitle Tests..." +echo "repo: $REPO_ROOT" +echo "dir : $SUB_DIR" +echo "port: $PORT" +echo "======================================" -# Check if server is running -echo "🔍 Checking if server is running..." -if curl -s http://localhost:3001/subtitles/health > /dev/null; then - echo "✅ Server is running on localhost:3001" -else - echo "❌ Server is not running. Please start it first:" - echo " bun src/server.ts" - echo " or" - echo " npm run start:server" - exit 1 -fi +if ! command -v pnpm >/dev/null 2>&1; then + echo "❌ pnpm not found. Install it to match this workspace’s packageManager." + exit 1 +fi + +echo "🔍 Checking server at ${HEALTH_URL}..." +if curl -sf "${HEALTH_URL}" >/dev/null; then + echo "✅ Server is running on localhost:${PORT}" +else + echo "❌ Server is not running. Start it first:" + echo " bun src/server.ts" + echo " or" + echo " pnpm run start:server" + exit 1 +fi echo -echo "🧪 Running Integration Tests..." -echo "================================" - -# Run the enhanced endpoints test -npm test -- test-enhanced-endpoints.test.ts +echo "🧪 Running Integration Tests..." +echo "================================" +pushd "$REPO_ROOT" >/dev/null +pnpm run test:subtitles:integration || INTEG_RC=$? || true +INTEG_RC="${INTEG_RC:-0}" +popd >/dev/null echo -echo "🎯 Running Unit Tests..." -echo "========================" - -# Run the unit tests (may have mocking issues but worth trying) -npm test -- subtitle-processor-enhanced.test.ts +echo "🎯 Running Unit Tests..." +echo "========================" +pushd "$REPO_ROOT" >/dev/null +pnpm run test:subtitles:unit || UNIT_RC=$? || true +UNIT_RC="${UNIT_RC:-0}" +popd >/dev/null echo -echo "🎉 Test run completed!" -echo "" -echo "📊 Summary:" -echo " - Integration tests: ✅ All working" -echo " - Unit tests: ⚠️ May have mocking issues" -echo " - Server status: ✅ Running" +echo "🎉 Test run completed!" +echo +echo "📊 Summary:" +if [[ "$INTEG_RC" -eq 0 ]]; then + echo " - Integration tests: ✅ Passed" +else + echo " - Integration tests: ❌ Failed (exit $INTEG_RC)" +fi +if [[ "$UNIT_RC" -eq 0 ]]; then + echo " - Unit tests : ✅ Passed" +else + echo " - Unit tests : ❌ Failed (exit $UNIT_RC)" +fi +echo " - Server status : ✅ Running" +echo +echo "💡 Watch mode: pnpm run test:subtitles:watch" +echo "💡 All subtitle tests: pnpm run test:subtitles" -echo "" -echo "💡 To run tests in watch mode:" -echo " npm run test:subtitles:watch" -echo "" -echo "💡 To run all subtitle tests:" -echo " npm run test:subtitles" +[[ "$INTEG_RC" -eq 0 && "$UNIT_RC" -eq 0 ]]Once applied, please re-run:
bash src/domain/media/subtitle-formats/run-tests.shand confirm that the summary accurately reflects test outcomes and the final exit code is non-zero if any suite fails.
src/domain/media/subtitle-formats/subtitle-processor-enhanced.test.ts (1)
18-28: test(mocking): good use of vi.mock with deferred importMock is declared before dynamic import, which ensures the stubbed
SubtitleConverterLive.convertis used. Nicely done.src/domain/media/subtitle-formats/subtitle-converter-clean.test.ts (1)
1-1: LGTM! Correct import from@effect/vitestThe test file correctly imports from
@effect/vitestas required by the coding guidelines for Effect-based tests.src/domain/media/subtitle-formats/endpoints.ts (1)
127-128: Return type missing for factory functionThe
createSubtitleApifunction should have an explicit return type.-export const createSubtitleApi = () => +export const createSubtitleApi = (): HttpApi.HttpApi => HttpApi.make('subtitleApi').add(subtitleGroup)Likely an incorrect or invalid review comment.
src/domain/media/subtitle-formats/subtitle-formats.schema.ts (1)
26-27: docs(schema): formats literal looks goodUsing a literal union for formats is aligned with downstream conversion logic.
| "test:subtitles": "vitest run src/domain/media/subtitle-formats/", | ||
| "test:subtitles:watch": "vitest src/domain/media/subtitle-formats/", |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
test(scripts): align subtitle test commands with pnpm and add focused unit/integration runners
Given the repo uses pnpm (packageManager is pnpm@10.7.0), it’s clearer if the runner script and docs call pnpm script aliases. Recommend adding explicit unit/integration scripts so run-tests.sh and README don’t hardcode test file names.
Apply this diff to extend scripts:
"scripts": {
"dev:server": "bun --inspect-wait --watch src/server.ts",
"start:server": "bun src/server.ts",
"test": "vitest run",
"test:watch": "vitest",
"test:ui": "vitest --ui",
"test:subtitles": "vitest run src/domain/media/subtitle-formats/",
"test:subtitles:watch": "vitest src/domain/media/subtitle-formats/",
+ "test:subtitles:integration": "vitest run src/domain/media/subtitle-formats/test-enhanced-endpoints.test.ts",
+ "test:subtitles:unit": "vitest run src/domain/media/subtitle-formats/subtitle-processor-enhanced.test.ts",
"check": "biome check --write .",
"typecheck": "tspc --noEmit",
"build": "tspc"
},This keeps CI stable and lets the shell script and README call pnpm run test:subtitles:* instead of invoking npm test -- ... directly.
📝 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.
| "test:subtitles": "vitest run src/domain/media/subtitle-formats/", | |
| "test:subtitles:watch": "vitest src/domain/media/subtitle-formats/", | |
| "scripts": { | |
| "dev:server": "bun --inspect-wait --watch src/server.ts", | |
| "start:server": "bun src/server.ts", | |
| "test": "vitest run", | |
| "test:watch": "vitest", | |
| "test:ui": "vitest --ui", | |
| "test:subtitles": "vitest run src/domain/media/subtitle-formats/", | |
| "test:subtitles:watch": "vitest src/domain/media/subtitle-formats/", | |
| "test:subtitles:integration": "vitest run src/domain/media/subtitle-formats/test-enhanced-endpoints.test.ts", | |
| "test:subtitles:unit": "vitest run src/domain/media/subtitle-formats/subtitle-processor-enhanced.test.ts", | |
| "check": "biome check --write .", | |
| "typecheck": "tspc --noEmit", | |
| "build": "tspc" | |
| }, |
🤖 Prompt for AI Agents
In package.json around lines 13-14, the current subtitle test scripts call
vitest directly and don't provide pnpm-friendly aliases or explicit
unit/integration runners; update the scripts section to (1) expose these scripts
as pnpm-invokable entries (so callers use `pnpm run test:subtitles:*`), and (2)
add explicit focused runners such as `test:subtitles:unit` and
`test:subtitles:integration` (and watch counterparts) that run vitest against
the appropriate unit or integration test globs/dirs; ensure names and semantics
match run-tests.sh and README so they no longer hardcode npm invocations or file
paths.
| import { describe, it, expect, vi } from 'vitest' | ||
| import { Effect as E } from 'effect' |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
test: consider @effect/vitest for consistency; remove unused Effect import
- tests here don’t run Effects, so vitest is acceptable; however, for repo-wide consistency you may import from @effect/vitest.
Effect as Eis imported but unused.
Apply this minimal diff:
-import { describe, it, expect, vi } from 'vitest'
-import { Effect as E } from 'effect'
+import { describe, it, expect, vi } from 'vitest'Or, for consistency:
-import { describe, it, expect, vi } from 'vitest'
+import { describe, it, expect, vi } from '@effect/vitest'📝 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.
| import { describe, it, expect, vi } from 'vitest' | |
| import { Effect as E } from 'effect' | |
| import { describe, it, expect, vi } from 'vitest' |
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/endpoints.test.ts around lines 1 to 2, the
test imports come from 'vitest' and also import `Effect as E` from 'effect'
which is unused; switch to the repo-consistent test helper by importing
describe, it, expect, vi from '@effect/vitest' and remove the unused `import {
Effect as E } from 'effect'` line so the file only imports the needed vitest
utilities from @effect/vitest.
| describe('Endpoint Configuration', () => { | ||
| it('should have correct HTTP methods', () => { | ||
| // The processSubtitles endpoint should be a POST endpoint | ||
| expect(subtitleGroup.endpoints).toBeDefined() | ||
| }) | ||
|
|
||
| it('should have proper error handling configured', () => { | ||
| // This test ensures the endpoints are properly configured | ||
| // The actual error handling is tested in the handler tests | ||
| expect(subtitleGroup).toBeDefined() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
test(endpoints): strengthen assertions on API group shape
Current checks only assert that properties exist. Consider asserting the group contains the expected endpoints and prefix to guard accidental regressions.
Example follow-up:
expect(subtitleGroup).toHaveProperty('prefix')
expect(String(subtitleGroup.prefix)).toContain('/subtitles')
expect(JSON.stringify(subtitleGroup.endpoints)).toMatch(/process|process-enhanced|formats|health/)This keeps the test lightweight while catching mis-wiring.
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/endpoints.test.ts around lines 160 to 171,
the tests only assert existence of subtitleGroup and its endpoints; strengthen
assertions to verify the group's prefix contains the expected '/subtitles' route
and that endpoints include the expected names (e.g. process, process-enhanced,
formats, health). Update the tests to assert subtitleGroup has a 'prefix'
property whose string contains '/subtitles' and that subtitleGroup.endpoints (or
its JSON/string form) matches the expected endpoint keys or patterns to catch
wiring regressions.
| export const addSubtitleEndpoints = (api: HttpApi.HttpApi) => | ||
| api.add(subtitleGroup) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Return type missing for helper function
The addSubtitleEndpoints function should have an explicit return type for better type safety.
-export const addSubtitleEndpoints = (api: HttpApi.HttpApi) =>
+export const addSubtitleEndpoints = (api: HttpApi.HttpApi): HttpApi.HttpApi =>
api.add(subtitleGroup)📝 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.
| export const addSubtitleEndpoints = (api: HttpApi.HttpApi) => | |
| api.add(subtitleGroup) | |
| export const addSubtitleEndpoints = (api: HttpApi.HttpApi): HttpApi.HttpApi => | |
| api.add(subtitleGroup) |
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/endpoints.ts around lines 120-121, the
helper function addSubtitleEndpoints lacks an explicit return type; change its
signature to include a clear TypeScript return type (e.g. : HttpApi.HttpApi) so
the function's return is type-safe and supports chaining. Update the function
declaration to declare that return type, ensure HttpApi is correctly
imported/available in the file, and adjust any consuming code if it relied on
implicit typing.
| ### Health Check | ||
| ```http | ||
| GET /subtitles/health | ||
| ``` | ||
| Returns service status and timestamp. |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
docs(markdown): fix markdownlint violations (blank lines around headings/fences, trailing spaces, EOF newline)
markdownlint-cli2 flagged multiple MD022/MD031/MD032/MD009/MD047 issues. Insert blank lines around headings and fenced blocks, and remove trailing whitespace. A few representative fixes are shown; please apply consistently across the file.
Apply representative diffs:
### Health Check
-```http
+```http
GET /subtitles/healthReturns service status and timestamp.
Get Supported Formats
-http +http
GET /subtitles/formats
Returns array of supported subtitle formats.
### Legacy Single Format Processing
-```http
+```http
POST /subtitles/process
Content-Type: application/json
#### **Option 1: Test Runner Script (Recommended)**
-```bash
+```bash
cd src/domain/media/subtitle-formats
./run-tests.sh
```diff
### Basic Single Format
-```typescript
+```typescript
import { enhancedProcessSubtitlesHandler } from './subtitle-processor-enhanced.handler'
const result = await enhancedProcessSubtitlesHandler({
### With Options
-```typescript
+```typescript
const result = await enhancedProcessSubtitlesHandler({ ### Validation Errors
-```typescript
+```typescript
try {-## 🔮 Future Enhancements
-
-- **Batch processing**: Process multiple subtitle files
+## 🔮 Future Enhancements
+
+- **Batch processing**: Process multiple subtitle files
...
- - **WebSocket support**: Real-time processing updates
+ - **WebSocket support**: Real-time processing updates
+After applying, run: pnpm dlx markdownlint-cli2 src/domain/media/subtitle-formats/README.md
Also applies to: 39-43, 45-49, 70-76, 100-106, 111-116, 121-126, 192-205, 206-215, 223-237, 241-252, 254-258, 261-267, 268-274, 282-288
🏁 Script executed:
#!/usr/bin/env bash
pnpm dlx markdownlint-cli2 src/domain/media/subtitle-formats/README.md || trueLength of output: 8207
docs(markdown): fix markdownlint violations in src/domain/media/subtitle-formats/README.md
Please address the 47 markdownlint errors reported by pnpm dlx markdownlint-cli2 src/domain/media/subtitle-formats/README.md. In particular:
- Add a blank line before and after every heading (MD022) and list (MD032).
- Surround each fenced code block with blank lines (MD031).
- Remove all trailing whitespace (MD009).
- Wrap or truncate lines to ≤ 80 characters (MD013), or explicitly disable that rule where longer lines are unavoidable.
- Ensure the file ends with exactly one newline (MD047).
After applying these changes file-wide, rerun:
pnpm dlx markdownlint-cli2 src/domain/media/subtitle-formats/README.mdto verify that all violations are resolved.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
33-33: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
34-34: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
36-36: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/README.md around lines 33 to 37, the file
currently triggers many markdownlint violations (MD022, MD032, MD031, MD009,
MD013, MD047); fix them by adding a blank line before and after each heading and
list, surrounding every fenced code block with blank lines, removing trailing
whitespace from all lines, wrapping or truncating lines to <=80 characters (or
add a rule-disable comment for unavoidable long lines), and ensure the file ends
with exactly one newline; after making these edits run pnpm dlx
markdownlint-cli2 src/domain/media/subtitle-formats/README.md to confirm all
violations are resolved.
| it('should fail with SubtitleFormatUnsupported for mixed valid/invalid formats', async () => { | ||
| const request = { | ||
| title: 'Mixed Valid/Invalid Test', | ||
| outputFormat: 'srt,invalid,vtt', | ||
| subtitleData: [ | ||
| { | ||
| start: 0, | ||
| end: 1000, | ||
| text: 'Test' | ||
| } | ||
| ] | ||
| } | ||
|
|
||
| await expect( | ||
| E.runPromise(enhancedProcessSubtitlesHandler(request)) | ||
| ).rejects.toThrow() | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
test(errors): assert mixed valid/invalid formats error as tagged
Mirror the previous change for the mixed-format case.
Apply this diff:
- await expect(
- E.runPromise(enhancedProcessSubtitlesHandler(request))
- ).rejects.toThrow()
+ const result = await E.runPromise(
+ enhancedProcessSubtitlesHandler(request).pipe(E.either)
+ )
+ expect(result._tag).toBe('Left')
+ // @ts-expect-error - runtime check
+ expect(result.left._tag).toBe('SubtitleFormatUnsupported')📝 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.
| it('should fail with SubtitleFormatUnsupported for mixed valid/invalid formats', async () => { | |
| const request = { | |
| title: 'Mixed Valid/Invalid Test', | |
| outputFormat: 'srt,invalid,vtt', | |
| subtitleData: [ | |
| { | |
| start: 0, | |
| end: 1000, | |
| text: 'Test' | |
| } | |
| ] | |
| } | |
| await expect( | |
| E.runPromise(enhancedProcessSubtitlesHandler(request)) | |
| ).rejects.toThrow() | |
| }) | |
| it('should fail with SubtitleFormatUnsupported for mixed valid/invalid formats', async () => { | |
| const request = { | |
| title: 'Mixed Valid/Invalid Test', | |
| outputFormat: 'srt,invalid,vtt', | |
| subtitleData: [ | |
| { | |
| start: 0, | |
| end: 1000, | |
| text: 'Test' | |
| } | |
| ] | |
| } | |
| const result = await E.runPromise( | |
| enhancedProcessSubtitlesHandler(request).pipe(E.either) | |
| ) | |
| expect(result._tag).toBe('Left') | |
| // @ts-expect-error - runtime check | |
| expect(result.left._tag).toBe('SubtitleFormatUnsupported') | |
| }) |
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-processor-enhanced.test.ts around
lines 135 to 151, the test currently uses a generic await
expect(...).rejects.toThrow() for the mixed valid/invalid formats case; change
it to assert the specific tagged error (SubtitleFormatUnsupported) like the
earlier single-format test: replace the generic rejects.toThrow() with an
assertion that the promise rejects with the SubtitleFormatUnsupported error
(matching by error class/name or message used elsewhere in the suite) so the
test explicitly verifies the tagged error for mixed-format input.
| // Mock conversion failure | ||
| mockConvert.mockReturnValue(E.fail( | ||
| new Error('Conversion failed') | ||
| )) | ||
|
|
||
| await expect( | ||
| E.runPromise(enhancedProcessSubtitlesHandler(request)) | ||
| ).rejects.toThrow() | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
test(errors): mock typed ConversionError and assert mapping to SubtitleConversionFailed
Currently the test mocks a generic Error. Mocking a typed ConversionError ensures the handler’s E.catchTags path is exercised and mapped correctly to SubtitleConversionFailed.
Apply this diff:
+import { ConversionError } from './subtitle-formats.errors'
@@
- // Mock conversion failure
- mockConvert.mockReturnValue(E.fail(
- new Error('Conversion failed')
- ))
+ // Mock typed conversion failure that the handler maps to SubtitleConversionFailed
+ mockConvert.mockReturnValue(
+ E.fail(new ConversionError({ format: 'srt' }))
+ )
@@
- await expect(
- E.runPromise(enhancedProcessSubtitlesHandler(request))
- ).rejects.toThrow()
+ const result = await E.runPromise(
+ enhancedProcessSubtitlesHandler(request).pipe(E.either)
+ )
+ expect(result._tag).toBe('Left')
+ // @ts-expect-error - runtime check
+ expect(result.left._tag).toBe('SubtitleConversionFailed')📝 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.
| // Mock conversion failure | |
| mockConvert.mockReturnValue(E.fail( | |
| new Error('Conversion failed') | |
| )) | |
| await expect( | |
| E.runPromise(enhancedProcessSubtitlesHandler(request)) | |
| ).rejects.toThrow() | |
| }) | |
| // at top of file, alongside other imports | |
| import { ConversionError } from './subtitle-formats.errors' | |
| // … rest of imports and setup … | |
| describe('enhancedProcessSubtitlesHandler', () => { | |
| // … earlier tests … | |
| it.effect('errors: maps ConversionError to SubtitleConversionFailed', async () => { | |
| // Mock typed conversion failure that the handler maps to SubtitleConversionFailed | |
| mockConvert.mockReturnValue( | |
| E.fail(new ConversionError({ format: 'srt' })) | |
| ) | |
| const result = await E.runPromise( | |
| enhancedProcessSubtitlesHandler(request).pipe(E.either) | |
| ) | |
| expect(result._tag).toBe('Left') | |
| // @ts-expect-error - runtime check of runtime error tag | |
| expect(result.left._tag).toBe('SubtitleConversionFailed') | |
| }) | |
| }) |
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-processor-enhanced.test.ts around
lines 251 to 259, the test currently mocks a generic Error for conversion
failure; change the mock to return a typed ConversionError (e.g. E.fail(new
ConversionError(...))) so the handler’s E.catchTags branch is exercised, and
update the assertion to expect the rejection to be a SubtitleConversionFailed
(e.g. assert the thrown error is an instance/type or message matching
SubtitleConversionFailed) instead of a generic toThrow.
| describe('Type Safety', () => { | ||
| it('should enforce type safety for subtitle data', () => { | ||
| // This test ensures TypeScript compilation works correctly | ||
| const validSubtitleItem: SubtitleItem = { | ||
| start: 0, | ||
| end: 1000, | ||
| text: 'Valid subtitle', | ||
| speaker: 1 | ||
| } | ||
|
|
||
| expect(validSubtitleItem.start).toBe(0) | ||
| expect(validSubtitleItem.end).toBe(1000) | ||
| expect(validSubtitleItem.text).toBe('Valid subtitle') | ||
| expect(validSubtitleItem.speaker).toBe(1) | ||
| }) | ||
|
|
||
| it('should enforce type safety for format enum', () => { | ||
| const validFormats: SubtitleFormat[] = ['json', 'srt', 'vtt', 'plain-text'] | ||
|
|
||
| validFormats.forEach(format => { | ||
| expect(['json', 'srt', 'vtt', 'plain-text']).toContain(format) | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
test(types): avoid runtime tests for compile-time-only guarantees
These assertions validate TypeScript-only properties at runtime. Prefer tsd (or a dedicated type test) for compile-time checks, or drop them.
Option A: move to tsd-based tests.
Option B: remove the block to keep the suite focused on runtime behaviour.
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-processor-enhanced.test.ts around
lines 336-359, the test block asserts TypeScript compile-time guarantees at
runtime (type safety for SubtitleItem and SubtitleFormat); remove this
runtime-only test block or move its intent into a tsd-based type test suite:
delete the entire "Type Safety" describe block from the Jest tests, or create
corresponding .test-d.ts tsd files that use expectType/expectAssignable for
SubtitleItem and SubtitleFormat and add tsd to devDependencies and CI.
| it('should test health check endpoint', async () => { | ||
| if (!serverRunning) { | ||
| console.log('⏭️ Skipping test - server not running') | ||
| return | ||
| } | ||
|
|
||
| console.log('1️⃣ Testing Health Check...') | ||
| const healthResponse = await fetch(`${BASE_URL}/subtitles/health`) | ||
| const healthData = await healthResponse.json() | ||
|
|
||
| expect(healthResponse.status).toBe(200) | ||
| expect(healthData.status).toBe('healthy') | ||
| expect(healthData.service).toBe('subtitle-processor') | ||
| expect(healthData.timestamp).toBeDefined() | ||
|
|
||
| console.log('✅ Health Check Response:', healthData) | ||
| console.log('Status:', healthResponse.status) | ||
| }) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Test cleanup needed in the lifecycle hooks
The afterAll hook is empty and the test skipping pattern in lines 28-31 could be improved. Consider using Vitest's built-in test skipping functionality.
- afterAll(() => {
- // Cleanup if needed
- })
+ afterAll(async () => {
+ // Add any necessary cleanup here
+ })
it('should test health check endpoint', async () => {
- if (!serverRunning) {
- console.log('⏭️ Skipping test - server not running')
- return
- }
+ it.skipIf(!serverRunning)('should test health check endpoint', async () => {Committable suggestion skipped: line range outside the PR's diff.
| it('should test all endpoints comprehensively', async () => { | ||
| if (!serverRunning) { | ||
| console.log('⏭️ Skipping test - server not running') | ||
| return | ||
| } | ||
|
|
||
| console.log('🎯 Running comprehensive endpoint test...') | ||
|
|
||
| // This test runs all the above tests in sequence | ||
| // The individual tests above will handle the assertions | ||
| console.log('🎉 All endpoint tests completed successfully!') | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Incomplete test implementation
This test appears to be a placeholder that doesn't add value. It should either be implemented with actual comprehensive testing logic or removed.
Either implement the comprehensive test properly or remove it entirely:
- it('should test all endpoints comprehensively', async () => {
- if (!serverRunning) {
- console.log('⏭️ Skipping test - server not running')
- return
- }
-
- console.log('🎯 Running comprehensive endpoint test...')
-
- // This test runs all the above tests in sequence
- // The individual tests above will handle the assertions
- console.log('🎉 All endpoint tests completed successfully!')
- })📝 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.
| it('should test all endpoints comprehensively', async () => { | |
| if (!serverRunning) { | |
| console.log('⏭️ Skipping test - server not running') | |
| return | |
| } | |
| console.log('🎯 Running comprehensive endpoint test...') | |
| // This test runs all the above tests in sequence | |
| // The individual tests above will handle the assertions | |
| console.log('🎉 All endpoint tests completed successfully!') | |
| }) | |
| // The comprehensive endpoint test placeholder has been removed. |
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/test-enhanced-endpoints.test.ts around
lines 362 to 373, the test labeled "should test all endpoints comprehensively"
is just a placeholder that logs messages and returns when serverRunning is
false; either implement it to run the actual sequence of endpoint tests and
assert results, or delete the test entirely to avoid meaningless coverage. If
implementing, invoke the existing endpoint test functions in sequence (awaiting
each), collect/propagate any assertion failures, and remove the console-only
branches; if removing, delete the entire it(...) block and any references to
this wrapper so the test suite relies on the individual tests already present.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/domain/media/subtitle-formats/subtitle-converter.ts (1)
1-745: chore: enforce consistent Schema import and consolidate duplicate helpers
- Replace incorrect
import type { Schema } from 'effect'with a real import alias in:
- src/domain/media/subtitle-formats/subtitle-converter.ts
- src/handlers/media/transcribe-workflow.handler.ts
- import type { Schema } from 'effect'
- import { Schema as S } from 'effect'
• Update all calls to `Schema.validate(...)` → `S.validate(...)`
Consolidate subtitle-timing helpers
We currently have two identical implementations:
applyTimingOffsetin subtitle-converter.tsaddTimingOffsetin subtitle-filters.ts and referenced in tests
Action:
• Remove one duplicate (preferapplyTimingOffset)
• Update subtitle-filters.ts, subtitle-pipeline-simple.ts and all tests to import & useapplyTimingOffsetonlyDRY up time-formatting utilities
The functionsformatTimeSrtandformatTimeVttare duplicated in:
- subtitle-converter.ts
- subtitle-filters.ts
- subtitle-pipeline-simple.ts
Action:
• Extract both into a new module, e.g. src/utils/time-formats.ts
• Import them where needed and remove their local definitionsEnsure all corresponding tests import from the updated locations. This refactor will eliminate runtime errors (due to the
type-only import) and reduce duplication across the codebase.
♻️ Duplicate comments (7)
src/domain/media/subtitle-formats/subtitle-converter.ts (7)
1-16: fix: replace invalid Effect Schema import; import schema values correctly
- Importing
type { Schema }and later using it as a value breaks at compile time.- Per repo rules, use
Schema as S.- Also,
SubtitleConversionResultSchemais a schema value, not a type; importing it withtypeis incorrect.- To use schema values alongside homonymous exported types (e.g.,
SubtitleJson), import the schema value under an alias.Apply:
-import { Effect as E, Option, Stream } from 'effect' -import type { Schema } from 'effect' +import { Effect as E, Option, Stream, Schema as S } from 'effect' @@ -import type { - ConversionOptions, - MultipleFormatResult, - SubtitleConversionResultSchema, - SubtitleFormat, - SubtitleItem, - SubtitleJson, -} from './subtitle-formats.schema' +import type { + ConversionOptions, + MultipleFormatResult, + SubtitleFormat, + SubtitleItem, + SubtitleJson, +} from './subtitle-formats.schema' +// schema values (runtime) — alias to avoid clashing with the homonymous types +import { + SubtitleJson as SubtitleJsonSchema, + SubtitleItem as SubtitleItemSchema, + SubtitleConversionResultSchema, +} from './subtitle-formats.schema'
152-165: refactor: de-duplicate timing offset logic; reuse shared filter
applyTimingOffsetduplicates single-item logic already present insubtitle-filters.ts(addTimingOffset). Centralise in one place to prevent drift.Proposed minimal change:
-/** - * Applies timing offset to subtitle items using generator for streaming processing - */ -export const applyTimingOffset = - (offset: number) => - (subtitle: SubtitleItem): SubtitleItem => ({ - ...subtitle, - start: Math.max(0, subtitle.start + offset), - end: Math.max(0, subtitle.end + offset), - }) +// Reuse canonical transformation from filters +import { addTimingOffset as applyTimingOffset } from './subtitle-filters'If you prefer to keep it here, delete the duplicate in the filters module and export from a single canonical location.
351-379: perf: validate items directly instead of wrapping each in an arrayWrapping each item in a one-element array to call
validateSubtitleData([item])is unnecessary overhead and obscures item-level failures. Decode a singleSubtitleItemwith Effect Schema.return Stream.fromIterable(subtitles).pipe( - Stream.mapEffect((item) => - validateSubtitleData([item], allowEmptyText).pipe(E.map((arr) => arr[0])), - ), + Stream.mapEffect((item) => + S.decodeUnknown(SubtitleItemSchema)(item).pipe( + E.mapError( + (cause) => + new InvalidSubtitleDataError({ + cause: cause instanceof Error ? cause : new Error(String(cause)), + }), + ), + ), + ),
495-515: refactor: extract time-format utilities into a shared module
formatTimeSrtandformatTimeVttare duplicated elsewhere. Extract tosrc/domain/media/subtitle-formats/subtitle-time-utils.tsand import here and in other consumers.Suggested extraction:
-export const formatTimeSrt = (ms: number): string => { +// subtitle-time-utils.ts +export const formatTimeSrt = (ms: number): string => { @@ -} +} @@ -export const formatTimeVtt = (ms: number): string => { +export const formatTimeVtt = (ms: number): string => { @@ -} +}Then:
+import { formatTimeSrt, formatTimeVtt } from './subtitle-time-utils'
660-678: docs: collapse duplicate JSDoc blocks forconvertThere are two consecutive doc blocks describing the same method. Keep a single authoritative block to avoid drift.
- /** - * Converts subtitle data to a specific format - * - * @param subtitles - Universal subtitle data (text with timing) - * @param format - Target format for conversion (json, srt, vtt, plain-text) - * @param options - Processing options to apply before conversion - * @returns Effect that succeeds with converted content or fails with conversion error - */
684-701: fix: schema validation usage and error construction are incorrect
Schema.validateis not the recommended API; useS.decodeUnknownto produce anEffect.SubtitleJsonSchemaisn’t imported; the current code won’t compile.InvalidSubtitleDataErrorrequires{ cause: Error }per your error type; passing a string breaks the tag constructor.- E.gen(function* () { - // Validate subtitles against the schema - const validationResult = Schema.validate(SubtitleJsonSchema, subtitles) - if (validationResult._tag === 'Left') { - throw new InvalidSubtitleDataError('Invalid subtitle data: ' + validationResult.left) - } - // Proceed with conversion if validation succeeds - return yield* convertSubtitleFormat(subtitles, format, options) - }).pipe( + E.gen(function* () { + // Validate against Effect Schema (returns Effect) + yield* S.decodeUnknown(SubtitleJsonSchema)(subtitles).pipe( + E.mapError( + (cause) => + new InvalidSubtitleDataError({ + cause: cause instanceof Error ? cause : new Error(String(cause)), + }), + ), + ) + // Proceed with conversion if validation succeeds + return yield* convertSubtitleFormat(subtitles, format, options) + }).pipe(
717-731: fix: incorrectSchemanamespace usage in type; align withSand ensure value importUse
S.Schema.Type<typeof SubtitleConversionResultSchema>and ensureSubtitleConversionResultSchemais imported as a value (addressed in the import fix).- const results: Schema.Schema.Type< - typeof SubtitleConversionResultSchema - >[] = [] + const results: S.Schema.Type<typeof SubtitleConversionResultSchema>[] = []
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/domain/media/subtitle-formats/subtitle-converter.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.ts
📄 CodeRabbit inference engine (.rules/code-style.mdc)
src/**/*.ts: Always use relative paths for internal imports; never usesrc/aliases for internal imports in TypeScript files.
Use package imports only for external dependencies in TypeScript files.
Avoid obvious comments; never add comments that just repeat what the code is doing. Only add comments for complex business logic that needs explanation.
Use JSDoc for public APIs, not inline comments for obvious operations.
Add comments only for complex business logic, non-obvious workarounds, edge cases, or important context that affects future changes.
UseEffect.Serviceclass extension pattern for services, include dependencies in service definition, and providemakeTestServicefor testing.
UseE.gen(function* () {})for complex flows; yield effects directly without helper functions; useyield*for effects that return values.
UseData.Taggedfor domain errors, create specific error types for different failure cases, and handle errors explicitly withE.catchTags.
Use Effect's logging utilities instead ofconsole.log; include relevant context in log messages and use appropriate log levels.
Add spans for observability usingE.withSpan, include relevant attributes, and use descriptive span names.
Use PascalCase for service classes (e.g.,BlogStore,SearchStore).
Use camelCase for functions and variables (e.g.,getBlogById,blogPost).
Use UPPERCASE for constants (e.g.,TABLE_NAME,DEFAULT_LIMIT).
Use descriptive names for functions and methods that indicate purpose.
Prefix usecases with action (e.g.,createBlogPostUsecase,getBlogByIdUsecase).
Prefix handlers with HTTP method context (e.g.,_handler,handler).
Use Effect Schema for all data validation; define schemas with explicit types and use branded types for domain identifiers.
Use branded types for domain identifiers and create factory functions with validation.
Access configuration through the Effect system, not directly viaprocess.env.
Use pipe syntax for effect composition;...
Files:
src/domain/media/subtitle-formats/subtitle-converter.ts
src/**/*
📄 CodeRabbit inference engine (.rules/code-style.mdc)
Use kebab-case for file names (e.g.,
blog.store.ts,get-blog-by-id.ts).
Files:
src/domain/media/subtitle-formats/subtitle-converter.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.rules/effect-service-architecture.mdc)
Always use relative paths for internal imports; never use
src/aliases for internal imports. Use package imports only for external dependencies.
Files:
src/domain/media/subtitle-formats/subtitle-converter.ts
🧠 Learnings (12)
📚 Learning: 2025-08-10T21:45:57.853Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/code-style.mdc:0-0
Timestamp: 2025-08-10T21:45:57.853Z
Learning: Applies to src/**/*.ts : Use Effect Schema for all data validation; define schemas with explicit types and use branded types for domain identifiers.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.ts
📚 Learning: 2025-08-10T21:48:12.331Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/schema-validation.mdc:0-0
Timestamp: 2025-08-10T21:48:12.331Z
Learning: Applies to **/*.types.ts|**/schema/**/*.ts|**/schemas/**/*.ts|**/*-schema.ts|**/*-schemas.ts|**/branded.ts|**/domain/**/*.ts : Always use `Schema as S` for Effect Schema imports
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.ts
📚 Learning: 2025-08-10T21:46:10.977Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/cursor-rule-maintainer.mdc:0-0
Timestamp: 2025-08-10T21:46:10.977Z
Learning: Applies to .cursor/rules/schema-validation.mdc : Add new Effect Schema patterns, validation improvements, branded type patterns, and error handling for schemas to `schema-validation.mdc`
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Use proper Effect error handling patterns in utility functions.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.ts
📚 Learning: 2025-08-10T21:48:12.331Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/schema-validation.mdc:0-0
Timestamp: 2025-08-10T21:48:12.331Z
Learning: Applies to **/schema/**/*.ts|**/schemas/**/*.ts|**/*-schema.ts|**/*-schemas.ts : Test schema validation with valid and invalid inputs using effect/vitest and Effect-based test patterns
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Use Effect-TS patterns for async operations in utility functions.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.ts
📚 Learning: 2025-08-10T21:45:28.136Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/api-client-patterns.mdc:0-0
Timestamp: 2025-08-10T21:45:28.136Z
Learning: Applies to **/*.api-client.ts : Import types with the `type` keyword when used only for typing; import schemas without `type` when used for encoding/decoding.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Always use `effect/vitest` for tests involving Effect-based code; import test functions (`describe`, `expect`, `it`) from `effect/vitest`, not regular `vitest`.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Use proper branded type constructors for mock data in tests.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.ts
📚 Learning: 2025-08-10T21:47:21.625Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-testing-patterns.mdc:0-0
Timestamp: 2025-08-10T21:47:21.625Z
Learning: Applies to **/*.test.ts : Use `it.effect` for tests that need TestContext (TestClock, etc.) in Effect-TS test files.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.ts
📚 Learning: 2025-08-10T21:46:54.925Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/effect-store-patterns.mdc:0-0
Timestamp: 2025-08-10T21:46:54.925Z
Learning: Applies to **/*.store.ts : Convert external errors (e.g., DynamoDB, S3) to domain errors using Effect-TS error handling patterns
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.ts
📚 Learning: 2025-08-10T21:45:57.853Z
Learnt from: CR
PR: BleedingDev/coursition-effect-experiment#0
File: .rules/code-style.mdc:0-0
Timestamp: 2025-08-10T21:45:57.853Z
Learning: Applies to src/**/*.ts : Access configuration through the Effect system, not directly via `process.env`.
Applied to files:
src/domain/media/subtitle-formats/subtitle-converter.ts
🧬 Code graph analysis (1)
src/domain/media/subtitle-formats/subtitle-converter.ts (2)
src/domain/media/subtitle-formats/subtitle-formats.errors.ts (4)
InvalidSubtitleDataError(37-41)InvalidTimingError(50-52)UnsupportedFormatError(43-48)ConversionError(54-57)src/domain/media/subtitle-formats/subtitle-formats.schema.ts (11)
SubtitleJson(21-21)SubtitleJson(97-97)SubtitleItem(7-16)SubtitleItem(96-96)ConversionOptions(31-42)ConversionOptions(99-99)SubtitleFormat(26-26)SubtitleFormat(98-98)SubtitleConversionResultSchema(54-57)MultipleFormatResult(59-61)MultipleFormatResult(100-102)
🔇 Additional comments (4)
src/domain/media/subtitle-formats/subtitle-converter.ts (4)
542-564: style: SRT generation is correct and readableBlock numbering, timing lines, and blank line separation are handled properly. Good use of helpers and spans.
572-593: style: VTT generation with header looks goodIncludes
WEBVTTheader, proper cue formatting, and observability span.
626-658: docs: solid, focused service descriptionGood separation of concerns and explicit non-goals. Clear examples and effect-aware usage.
396-440: nit: preferE.catchTagsfor domain errors inconvertSubtitleFormatThe function returns domain-tagged errors; consider
E.catchTagswhere you need targeted handling, especially forUnsupportedFormatError, to enrich logs/spans.No code change required; just a recommendation to keep error handling idiomatic to Effect.
⛔ Skipped due to learnings
Learnt from: CR PR: BleedingDev/coursition-effect-experiment#0 File: .rules/code-style.mdc:0-0 Timestamp: 2025-08-10T21:45:57.853Z Learning: Applies to src/**/*.ts : Use `Data.Tagged` for domain errors, create specific error types for different failure cases, and handle errors explicitly with `E.catchTags`.Learnt from: CR PR: BleedingDev/coursition-effect-experiment#0 File: .rules/effect-store-patterns.mdc:0-0 Timestamp: 2025-08-10T21:46:54.925Z Learning: Applies to **/*.store.ts : Convert external errors (e.g., DynamoDB, S3) to domain errors using Effect-TS error handling patternsLearnt from: CR PR: BleedingDev/coursition-effect-experiment#0 File: .rules/effect-store-patterns.mdc:0-0 Timestamp: 2025-08-10T21:46:54.925Z Learning: Applies to **/*-store.ts : Convert external errors (e.g., DynamoDB, S3) to domain errors using Effect-TS error handling patternsLearnt from: CR PR: BleedingDev/coursition-effect-experiment#0 File: .rules/effect-store-patterns.mdc:0-0 Timestamp: 2025-08-10T21:46:54.925Z Learning: Applies to **/stores/**/*.ts : Convert external errors (e.g., DynamoDB, S3) to domain errors using Effect-TS error handling patternsLearnt from: CR PR: BleedingDev/coursition-effect-experiment#0 File: .rules/effect-testing-patterns.mdc:0-0 Timestamp: 2025-08-10T21:47:21.625Z Learning: Never use `throw new Error()` in Effect-TS applications; always create and use `Data.TaggedError` for error handling.Learnt from: CR PR: BleedingDev/coursition-effect-experiment#0 File: .rules/effect-service-architecture.mdc:0-0 Timestamp: 2025-08-10T21:46:38.101Z Learning: Use tagged errors for type-safe error handling. Convert external errors to domain errors at store boundaries, add context and logging at the usecase level, and map to appropriate HTTP responses at the handler level.Learnt from: CR PR: BleedingDev/coursition-effect-experiment#0 File: .rules/effect-handler-patterns.mdc:0-0 Timestamp: 2025-08-10T21:46:24.385Z Learning: Applies to **/lambdas/**/handler.ts,**/*-handler.ts : Handlers must map domain errors to HTTP responses using 'E.catchTags' with appropriate error-to-response mappings.Learnt from: CR PR: BleedingDev/coursition-effect-experiment#0 File: .rules/effect-testing-patterns.mdc:0-0 Timestamp: 2025-08-10T21:47:21.625Z Learning: Applies to **/*.test.ts : Test tagged errors with proper Effect patterns (using `E.exit` and `getExitError`).Learnt from: CR PR: BleedingDev/coursition-effect-experiment#0 File: .rules/schema-validation.mdc:0-0 Timestamp: 2025-08-10T21:48:12.331Z Learning: Applies to **/*.types.ts|**/schema/**/*.ts|**/schemas/**/*.ts|**/*-schema.ts|**/*-schemas.ts|**/branded.ts|**/domain/**/*.ts : Use `Data.TaggedError` for domain-specific error types and include relevant identifiers and context in error dataLearnt from: CR PR: BleedingDev/coursition-effect-experiment#0 File: .rules/effect-testing-patterns.mdc:0-0 Timestamp: 2025-08-10T21:47:21.625Z Learning: Use proper Effect error handling patterns in utility functions.
| E.tapError(E.logError), | ||
| E.withSpan('validateSubtitleData', { | ||
| attributes: { | ||
| count: Array.isArray(subtitles) ? subtitles.length : 0, | ||
| hasOptions: allowEmptyText !== undefined, | ||
| }, | ||
| }), | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
chore: correct span attribute; current expression is always true
hasOptions: allowEmptyText !== undefined is always true because allowEmptyText has a default. Use the boolean flag directly or rename for clarity.
- hasOptions: allowEmptyText !== undefined,
+ allowEmptyText,📝 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.
| E.tapError(E.logError), | |
| E.withSpan('validateSubtitleData', { | |
| attributes: { | |
| count: Array.isArray(subtitles) ? subtitles.length : 0, | |
| hasOptions: allowEmptyText !== undefined, | |
| }, | |
| }), | |
| ) | |
| E.tapError(E.logError), | |
| E.withSpan('validateSubtitleData', { | |
| attributes: { | |
| count: Array.isArray(subtitles) ? subtitles.length : 0, | |
| allowEmptyText, | |
| }, | |
| }), |
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-converter.ts around lines 143 to
150, the span attribute uses `hasOptions: allowEmptyText !== undefined` which is
always true due to the default parameter; replace that expression with a boolean
or clearer name: either set `hasOptions: !!allowEmptyText` to reflect its
truthiness, or rename the attribute to `hasAllowEmptyText: allowEmptyText`
(preferably the latter for clarity) so the span accurately reports whether the
allowEmptyText flag is enabled.
| /** | ||
| * Merges adjacent subtitles that are close in timing using generator for streaming processing | ||
| * | ||
| * @param subtitles - Array of subtitle items to merge | ||
| * @param threshold - Maximum gap in milliseconds to consider subtitles adjacent | ||
| * @returns Effect that succeeds with merged subtitles or fails with processing error | ||
| */ | ||
| export const mergeAdjacentSubtitles = ( | ||
| subtitles: SubtitleItem[], | ||
| threshold: number, | ||
| ) => { | ||
| if (subtitles.length <= 1) { | ||
| return E.succeed(subtitles) | ||
| } | ||
|
|
||
| const merged: SubtitleItem[] = [] | ||
| const first = subtitles[0] | ||
| if (!first) { | ||
| return E.succeed(subtitles) | ||
| } | ||
| let current: SubtitleItem = { | ||
| start: first.start, | ||
| end: first.end, | ||
| text: first.text, | ||
| speaker: first.speaker, | ||
| } | ||
|
|
||
| // Process subtitles one by one | ||
| for (let i = 1; i < subtitles.length; i++) { | ||
| const next = subtitles[i] | ||
| if (!next) { | ||
| continue | ||
| } | ||
|
|
||
| const gap = next.start - current.end | ||
|
|
||
| if (gap <= threshold) { | ||
| // Merge subtitles | ||
| const mergedSubtitle: SubtitleItem = { | ||
| start: current.start, | ||
| end: next.end, | ||
| text: `${current.text} ${next.text}`, | ||
| speaker: current.speaker === next.speaker ? current.speaker : undefined, | ||
| } | ||
| current = mergedSubtitle | ||
| } else { | ||
| // Add current to merged array and start new current | ||
| merged.push(current) | ||
| current = { | ||
| start: next.start, | ||
| end: next.end, | ||
| text: next.text, | ||
| speaker: next.speaker, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Add the last subtitle | ||
| merged.push(current) | ||
|
|
||
| return E.succeed(merged).pipe( | ||
| E.tapError(E.logError), | ||
| E.withSpan('mergeAdjacentSubtitles', { | ||
| attributes: { | ||
| originalCount: subtitles.length, | ||
| threshold, | ||
| }, | ||
| }), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
behaviour: merging concatenates with a space; consider preserving line breaks
For adjacent cues, preserving a newline tends to read better in SRT/VTT outputs. Optional, but improves readability.
- text: `${current.text} ${next.text}`,
+ text: `${current.text}\n${next.text}`,📝 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.
| /** | |
| * Merges adjacent subtitles that are close in timing using generator for streaming processing | |
| * | |
| * @param subtitles - Array of subtitle items to merge | |
| * @param threshold - Maximum gap in milliseconds to consider subtitles adjacent | |
| * @returns Effect that succeeds with merged subtitles or fails with processing error | |
| */ | |
| export const mergeAdjacentSubtitles = ( | |
| subtitles: SubtitleItem[], | |
| threshold: number, | |
| ) => { | |
| if (subtitles.length <= 1) { | |
| return E.succeed(subtitles) | |
| } | |
| const merged: SubtitleItem[] = [] | |
| const first = subtitles[0] | |
| if (!first) { | |
| return E.succeed(subtitles) | |
| } | |
| let current: SubtitleItem = { | |
| start: first.start, | |
| end: first.end, | |
| text: first.text, | |
| speaker: first.speaker, | |
| } | |
| // Process subtitles one by one | |
| for (let i = 1; i < subtitles.length; i++) { | |
| const next = subtitles[i] | |
| if (!next) { | |
| continue | |
| } | |
| const gap = next.start - current.end | |
| if (gap <= threshold) { | |
| // Merge subtitles | |
| const mergedSubtitle: SubtitleItem = { | |
| start: current.start, | |
| end: next.end, | |
| text: `${current.text} ${next.text}`, | |
| speaker: current.speaker === next.speaker ? current.speaker : undefined, | |
| } | |
| current = mergedSubtitle | |
| } else { | |
| // Add current to merged array and start new current | |
| merged.push(current) | |
| current = { | |
| start: next.start, | |
| end: next.end, | |
| text: next.text, | |
| speaker: next.speaker, | |
| } | |
| } | |
| } | |
| // Add the last subtitle | |
| merged.push(current) | |
| return E.succeed(merged).pipe( | |
| E.tapError(E.logError), | |
| E.withSpan('mergeAdjacentSubtitles', { | |
| attributes: { | |
| originalCount: subtitles.length, | |
| threshold, | |
| }, | |
| }), | |
| ) | |
| } | |
| export const mergeAdjacentSubtitles = ( | |
| subtitles: SubtitleItem[], | |
| threshold: number, | |
| ) => { | |
| if (subtitles.length <= 1) { | |
| return E.succeed(subtitles) | |
| } | |
| const merged: SubtitleItem[] = [] | |
| const first = subtitles[0] | |
| if (!first) { | |
| return E.succeed(subtitles) | |
| } | |
| let current: SubtitleItem = { | |
| start: first.start, | |
| end: first.end, | |
| text: first.text, | |
| speaker: first.speaker, | |
| } | |
| // Process subtitles one by one | |
| for (let i = 1; i < subtitles.length; i++) { | |
| const next = subtitles[i] | |
| if (!next) { | |
| continue | |
| } | |
| const gap = next.start - current.end | |
| if (gap <= threshold) { | |
| // Merge subtitles | |
| const mergedSubtitle: SubtitleItem = { | |
| start: current.start, | |
| end: next.end, | |
| text: `${current.text}\n${next.text}`, | |
| speaker: current.speaker === next.speaker ? current.speaker : undefined, | |
| } | |
| current = mergedSubtitle | |
| } else { | |
| // Add current to merged array and start new current | |
| merged.push(current) | |
| current = { | |
| start: next.start, | |
| end: next.end, | |
| text: next.text, | |
| speaker: next.speaker, | |
| } | |
| } | |
| } | |
| // Add the last subtitle | |
| merged.push(current) | |
| return E.succeed(merged).pipe( | |
| E.tapError(E.logError), | |
| E.withSpan('mergeAdjacentSubtitles', { | |
| attributes: { | |
| originalCount: subtitles.length, | |
| threshold, | |
| }, | |
| }), | |
| ) | |
| } |
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-converter.ts around lines 200 to
269, the merge logic currently concatenates adjacent subtitle texts with a
single space which loses original line breaks and reduces readability in SRT/VTT
outputs; change the merge to preserve/newline separate cues by joining texts
with '\n' (optionally trimming trailing/leading whitespace on parts to avoid
double spaces/newlines) when building mergedSubtitle and when initializing
current text for new groups, keeping speaker merge logic unchanged.
| // Helper function to process a single subtitle item | ||
| const processSingleSubtitle = ( | ||
| item: SubtitleItem, | ||
| options?: ConversionOptions, | ||
| ): SubtitleItem => { | ||
| let processedItem = item | ||
|
|
||
| // 1. Apply timing offset first | ||
| if (options?.timingOffset) { | ||
| processedItem = applyTimingOffset(options.timingOffset)(processedItem) | ||
| } | ||
|
|
||
| // 2. Clean text second | ||
| if (options?.cleanText !== false) { | ||
| // Default to true | ||
| processedItem = cleanSubtitleText(processedItem) | ||
| } | ||
|
|
||
| // 3. Add speaker info last | ||
| if (options?.includeSpeaker) { | ||
| processedItem = addSpeakerInfo(true)(processedItem) | ||
| } | ||
|
|
||
| return processedItem | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
nit: timing offset guard should check for defined, not truthy
if (options?.timingOffset) skips when 0 is provided. While a zero offset is a no-op, make the intent explicit and future-proof by checking for definedness.
- if (options?.timingOffset) {
+ if (options?.timingOffset !== undefined) {
processedItem = applyTimingOffset(options.timingOffset)(processedItem)
}📝 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.
| // Helper function to process a single subtitle item | |
| const processSingleSubtitle = ( | |
| item: SubtitleItem, | |
| options?: ConversionOptions, | |
| ): SubtitleItem => { | |
| let processedItem = item | |
| // 1. Apply timing offset first | |
| if (options?.timingOffset) { | |
| processedItem = applyTimingOffset(options.timingOffset)(processedItem) | |
| } | |
| // 2. Clean text second | |
| if (options?.cleanText !== false) { | |
| // Default to true | |
| processedItem = cleanSubtitleText(processedItem) | |
| } | |
| // 3. Add speaker info last | |
| if (options?.includeSpeaker) { | |
| processedItem = addSpeakerInfo(true)(processedItem) | |
| } | |
| return processedItem | |
| } | |
| // Helper function to process a single subtitle item | |
| const processSingleSubtitle = ( | |
| item: SubtitleItem, | |
| options?: ConversionOptions, | |
| ): SubtitleItem => { | |
| let processedItem = item | |
| // 1. Apply timing offset first | |
| if (options?.timingOffset !== undefined) { | |
| processedItem = applyTimingOffset(options.timingOffset)(processedItem) | |
| } | |
| // 2. Clean text second | |
| if (options?.cleanText !== false) { | |
| // Default to true | |
| processedItem = cleanSubtitleText(processedItem) | |
| } | |
| // 3. Add speaker info last | |
| if (options?.includeSpeaker) { | |
| processedItem = addSpeakerInfo(true)(processedItem) | |
| } | |
| return processedItem | |
| } |
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-converter.ts around lines 278 to
303, the guard `if (options?.timingOffset)` incorrectly treats 0 as falsy and
will skip applying a zero offset; change the check to test for definedness
(e.g., `options?.timingOffset !== undefined` or `options?.timingOffset != null`)
so a 0 offset is handled explicitly, ensuring applyTimingOffset is invoked when
timingOffset is provided even if it is zero.
| export const processSubtitlesStream = ( | ||
| subtitles: SubtitleJson, | ||
| options?: ConversionOptions, | ||
| ) => { | ||
| // Validate input data first (allow empty text if cleanText is enabled) | ||
| const allowEmptyText = options?.cleanText === true | ||
| return Stream.fromIterable(subtitles).pipe( | ||
| Stream.mapEffect((item) => | ||
| validateSubtitleData([item], allowEmptyText).pipe(E.map((arr) => arr[0])), | ||
| ), | ||
| // Apply transformations in parallel | ||
| Stream.map((item) => { | ||
| let processedItem = item | ||
| if (options?.timingOffset) { | ||
| processedItem = applyTimingOffset(options.timingOffset)(processedItem) | ||
| } | ||
| if (options?.cleanText !== false) { | ||
| processedItem = cleanSubtitleText(processedItem) | ||
| } | ||
| if (options?.includeSpeaker) { | ||
| processedItem = addSpeakerInfo(true)(processedItem) | ||
| } | ||
| return processedItem | ||
| }), | ||
| // Filter out empty text if cleanText is enabled | ||
| options?.cleanText === true | ||
| ? Stream.filter((item) => item.text.trim().length > 0) | ||
| : (s) => s, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
consistency: streaming contract vs PR objective
Both processSubtitlesStream and convertSubtitleFormatStream should accept/return true streams (generator/async iterator in, Stream<string> out). With the proposed changes above, the output side is addressed; consider adding overloads to accept AsyncIterable<SubtitleItem> to fully meet the “accepts a stream” objective.
Happy to provide a follow-up diff adding an overload:
processSubtitlesStream(input: AsyncIterable<SubtitleItem> | SubtitleJson, options?: ConversionOptions): Stream<SubtitleItem>- Normalise
AsyncIterableviaStream.fromAsyncIterable.
Also applies to: 444-474
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-converter.ts around lines 351 to
379 (and similarly adjust lines 444–474), the function currently only accepts a
SubtitleJson array input; add overloads so it also accepts
AsyncIterable<SubtitleItem> and normalize that input into a Stream using
Stream.fromAsyncIterable (or Stream.fromIterable for arrays) so the function
signature becomes processSubtitlesStream(input: AsyncIterable<SubtitleItem> |
SubtitleJson, options?: ConversionOptions): Stream<SubtitleItem>; update
internal code to branch on input type to create the base Stream, keep the
existing pipeline intact, and ensure the exported return type is
Stream<SubtitleItem> to satisfy the streaming contract.
🧹 Nitpick (assertive)
observability: add a span around stream processing
Other functions include spans; processSubtitlesStream currently has none. Add a withSpan (e.g., via Stream.unwrapScoped or wrapping the validation mapEffect) to provide parity in tracing.
Example:
- return Stream.fromIterable(subtitles).pipe(
+ return Stream.fromIterable(subtitles).pipe(
+ // annotate processing
+ Stream.tap(() =>
+ E.withSpan('processSubtitlesStream', {
+ attributes: { count: subtitles.length, hasOptions: options !== undefined },
+ })(E.unit),
+ ),Use any Stream-friendly pattern your team prefers for spans.
📝 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.
| export const processSubtitlesStream = ( | |
| subtitles: SubtitleJson, | |
| options?: ConversionOptions, | |
| ) => { | |
| // Validate input data first (allow empty text if cleanText is enabled) | |
| const allowEmptyText = options?.cleanText === true | |
| return Stream.fromIterable(subtitles).pipe( | |
| Stream.mapEffect((item) => | |
| validateSubtitleData([item], allowEmptyText).pipe(E.map((arr) => arr[0])), | |
| ), | |
| // Apply transformations in parallel | |
| Stream.map((item) => { | |
| let processedItem = item | |
| if (options?.timingOffset) { | |
| processedItem = applyTimingOffset(options.timingOffset)(processedItem) | |
| } | |
| if (options?.cleanText !== false) { | |
| processedItem = cleanSubtitleText(processedItem) | |
| } | |
| if (options?.includeSpeaker) { | |
| processedItem = addSpeakerInfo(true)(processedItem) | |
| } | |
| return processedItem | |
| }), | |
| // Filter out empty text if cleanText is enabled | |
| options?.cleanText === true | |
| ? Stream.filter((item) => item.text.trim().length > 0) | |
| : (s) => s, | |
| ) | |
| export const processSubtitlesStream = ( | |
| subtitles: SubtitleJson, | |
| options?: ConversionOptions, | |
| ) => { | |
| // Validate input data first (allow empty text if cleanText is enabled) | |
| const allowEmptyText = options?.cleanText === true | |
| return Stream.fromIterable(subtitles).pipe( | |
| // annotate processing | |
| Stream.tap(() => | |
| E.withSpan('processSubtitlesStream', { | |
| attributes: { count: subtitles.length, hasOptions: options !== undefined }, | |
| })(E.unit), | |
| ), | |
| Stream.mapEffect((item) => | |
| validateSubtitleData([item], allowEmptyText).pipe(E.map((arr) => arr[0])), | |
| ), | |
| // Apply transformations in parallel | |
| Stream.map((item) => { | |
| let processedItem = item | |
| if (options?.timingOffset) { | |
| processedItem = applyTimingOffset(options.timingOffset)(processedItem) | |
| } | |
| if (options?.cleanText !== false) { | |
| processedItem = cleanSubtitleText(processedItem) | |
| } | |
| if (options?.includeSpeaker) { | |
| processedItem = addSpeakerInfo(true)(processedItem) | |
| } | |
| return processedItem | |
| }), | |
| // Filter out empty text if cleanText is enabled | |
| options?.cleanText === true | |
| ? Stream.filter((item) => item.text.trim().length > 0) | |
| : (s) => s, | |
| ) | |
| } |
| export const convertSubtitleFormatStream = ( | ||
| subtitles: SubtitleJson, | ||
| format: SubtitleFormat, | ||
| options?: ConversionOptions, | ||
| ) => | ||
| processSubtitlesStream(subtitles, options).pipe( | ||
| Stream.runCollect, | ||
| E.map((chunk) => Array.from(chunk)), | ||
| E.flatMap((arr) => { | ||
| switch (format) { | ||
| case 'json': | ||
| return convertToJson(arr) | ||
| case 'srt': | ||
| return convertToSrt(arr) | ||
| case 'vtt': | ||
| return convertToVtt(arr) | ||
| case 'plain-text': | ||
| return convertToPlainText(arr) | ||
| default: | ||
| return E.fail( | ||
| new ConversionError({ | ||
| format: String(format), | ||
| cause: new UnsupportedFormatError({ | ||
| format, | ||
| supportedFormats: ['json', 'srt', 'vtt', 'plain-text'], | ||
| }), | ||
| }), | ||
| ) | ||
| } | ||
| }), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
feat: return a real stream of incremental output; avoid runCollect
The current implementation collects the stream into an array and returns a single Effect<string>, defeating incremental streaming. Return Stream<string> and emit format-specific chunks as items arrive. This aligns with the PR goal (“produces incremental, valid subtitle output”).
-export const convertSubtitleFormatStream = (
+export const convertSubtitleFormatStream = (
subtitles: SubtitleJson,
format: SubtitleFormat,
options?: ConversionOptions,
) =>
- processSubtitlesStream(subtitles, options).pipe(
- Stream.runCollect,
- E.map((chunk) => Array.from(chunk)),
- E.flatMap((arr) => {
- switch (format) {
- case 'json':
- return convertToJson(arr)
- case 'srt':
- return convertToSrt(arr)
- case 'vtt':
- return convertToVtt(arr)
- case 'plain-text':
- return convertToPlainText(arr)
- default:
- return E.fail(
- new ConversionError({
- format: String(format),
- cause: new UnsupportedFormatError({
- format,
- supportedFormats: ['json', 'srt', 'vtt', 'plain-text'],
- }),
- }),
- )
- }
- }),
- )
+ {
+ const processed$ = processSubtitlesStream(subtitles, options)
+ switch (format) {
+ case 'json': {
+ // Stream a valid JSON array: [item0,item1,...]
+ const body$ = processed$.pipe(
+ Stream.map((item) => JSON.stringify(item)),
+ Stream.mapAccum(false as boolean, (seen, s) => [true, (seen ? ',' : '') + s]),
+ )
+ return Stream.concat(Stream.succeed('['), Stream.concat(body$, Stream.succeed(']')))
+ }
+ case 'srt': {
+ return processed$.pipe(
+ Stream.mapAccum(1, (index, item) => [
+ index + 1,
+ `${index}\n${formatTimeSrt(item.start)} --> ${formatTimeSrt(item.end)}\n${item.text}\n\n`,
+ ]),
+ )
+ }
+ case 'vtt': {
+ const body$ = processed$.pipe(
+ Stream.map(
+ (item) =>
+ `${formatTimeVtt(item.start)} --> ${formatTimeVtt(item.end)}\n${item.text}\n\n`,
+ ),
+ )
+ return Stream.concat(Stream.succeed('WEBVTT\n\n'), body$)
+ }
+ case 'plain-text': {
+ return processed$.pipe(Stream.map((item) => `${item.text}\n\n`))
+ }
+ default: {
+ return Stream.fail(
+ new ConversionError({
+ format: String(format),
+ cause: new UnsupportedFormatError({
+ format,
+ supportedFormats: ['json', 'srt', 'vtt', 'plain-text'],
+ }),
+ }),
+ )
+ }
+ }
+ }📝 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.
| export const convertSubtitleFormatStream = ( | |
| subtitles: SubtitleJson, | |
| format: SubtitleFormat, | |
| options?: ConversionOptions, | |
| ) => | |
| processSubtitlesStream(subtitles, options).pipe( | |
| Stream.runCollect, | |
| E.map((chunk) => Array.from(chunk)), | |
| E.flatMap((arr) => { | |
| switch (format) { | |
| case 'json': | |
| return convertToJson(arr) | |
| case 'srt': | |
| return convertToSrt(arr) | |
| case 'vtt': | |
| return convertToVtt(arr) | |
| case 'plain-text': | |
| return convertToPlainText(arr) | |
| default: | |
| return E.fail( | |
| new ConversionError({ | |
| format: String(format), | |
| cause: new UnsupportedFormatError({ | |
| format, | |
| supportedFormats: ['json', 'srt', 'vtt', 'plain-text'], | |
| }), | |
| }), | |
| ) | |
| } | |
| }), | |
| ) | |
| export const convertSubtitleFormatStream = ( | |
| subtitles: SubtitleJson, | |
| format: SubtitleFormat, | |
| options?: ConversionOptions, | |
| ) => { | |
| const processed$ = processSubtitlesStream(subtitles, options) | |
| switch (format) { | |
| case 'json': { | |
| // Stream a valid JSON array: [item0,item1,...] | |
| const body$ = processed$.pipe( | |
| Stream.map((item) => JSON.stringify(item)), | |
| Stream.mapAccum(false as boolean, (seen, s) => [true, (seen ? ',' : '') + s]), | |
| ) | |
| return Stream.concat( | |
| Stream.succeed('['), | |
| Stream.concat(body$, Stream.succeed(']')), | |
| ) | |
| } | |
| case 'srt': { | |
| return processed$.pipe( | |
| Stream.mapAccum(1, (index, item) => [ | |
| index + 1, | |
| `${index}\n${formatTimeSrt(item.start)} --> ${formatTimeSrt(item.end)}\n${item.text}\n\n`, | |
| ]), | |
| ) | |
| } | |
| case 'vtt': { | |
| const body$ = processed$.pipe( | |
| Stream.map( | |
| (item) => | |
| `${formatTimeVtt(item.start)} --> ${formatTimeVtt(item.end)}\n${item.text}\n\n`, | |
| ), | |
| ) | |
| return Stream.concat(Stream.succeed('WEBVTT\n\n'), body$) | |
| } | |
| case 'plain-text': { | |
| return processed$.pipe( | |
| Stream.map((item) => `${item.text}\n\n`), | |
| ) | |
| } | |
| default: { | |
| return Stream.fail( | |
| new ConversionError({ | |
| format: String(format), | |
| cause: new UnsupportedFormatError({ | |
| format, | |
| supportedFormats: ['json', 'srt', 'vtt', 'plain-text'], | |
| }), | |
| }), | |
| ) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/domain/media/subtitle-formats/subtitle-converter.ts around lines 444 to
474, the current implementation collects the entire subtitle stream with
Stream.runCollect and returns a single Effect, preventing incremental output;
replace that collection with a streaming pipeline that yields format-specific
strings as items arrive. Concretely: remove Stream.runCollect and the downstream
Effect/E.map/E.flatMap usage and instead pipe processSubtitlesStream(subtitles,
options) into Stream.map or Stream.flatMap so each incoming chunk (use
Array.from(chunk) if conversion helpers expect arrays) is converted immediately
with convertToJson/convertToSrt/convertToVtt/convertToPlainText and emitted as
Stream<string>; for the unsupported-format branch return a failing Stream (e.g.,
Stream.fail with the ConversionError) so the function's return type becomes
Stream<string> producing incremental output.
| export const runSubtitleConversionStream = ( | ||
| subtitles: SubtitleJson, | ||
| format: SubtitleFormat, | ||
| options?: ConversionOptions, | ||
| ) => | ||
| convertSubtitleFormatStream(subtitles, format, options).pipe( | ||
| E.catchAll((err) => E.succeed({ error: err })), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
fix: run the stream in the endpoint; fold to string while preserving streaming API
After returning a Stream<string> from convertSubtitleFormatStream, this endpoint must execute it. Fold chunks into the final string (or keep as-is and expose the stream upstream).
export const runSubtitleConversionStream = (
subtitles: SubtitleJson,
format: SubtitleFormat,
options?: ConversionOptions,
) =>
- convertSubtitleFormatStream(subtitles, format, options).pipe(
- E.catchAll((err) => E.succeed({ error: err })),
- )
+ convertSubtitleFormatStream(subtitles, format, options).pipe(
+ Stream.runFold('', (acc, chunk) => acc + chunk),
+ E.catchAll((err) => E.succeed({ error: err })),
+ )📝 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.
| export const runSubtitleConversionStream = ( | |
| subtitles: SubtitleJson, | |
| format: SubtitleFormat, | |
| options?: ConversionOptions, | |
| ) => | |
| convertSubtitleFormatStream(subtitles, format, options).pipe( | |
| E.catchAll((err) => E.succeed({ error: err })), | |
| ) | |
| export const runSubtitleConversionStream = ( | |
| subtitles: SubtitleJson, | |
| format: SubtitleFormat, | |
| options?: ConversionOptions, | |
| ) => | |
| convertSubtitleFormatStream(subtitles, format, options).pipe( | |
| Stream.runFold('', (acc, chunk) => acc + chunk), | |
| E.catchAll((err) => E.succeed({ error: err })), | |
| ) |
#16
Streaming Subtitle Conversion Feature
Overview
This Merge Request introduces support for streaming subtitle conversion. The new functionality enables the system to accept a stream of subtitle items and begin generating valid subtitle output (SRT, VTT, plain text, etc.) in real time, as the video is being transcribed. This allows users to receive subtitle data incrementally, improving responsiveness and user experience for long or live videos.
Why Incremental Subtitles?
While most users typically want the complete, finalized subtitle file (e.g., for download, archiving, or sharing), there are important scenarios where incremental or partial subtitle output is valuable:
For most users, the default remains to wait for the whole subtitle file. The streaming feature is optional and designed to enhance flexibility for advanced or real-time use cases.
Motivation
Key Features
Usage Example
API Changes
runSubtitleProcessingStreamandrunSubtitleConversionStreamnow accept streams/generators and output incremental results.Testing
Documentation
Checklist
This feature enables real-time subtitle delivery and is ready for review and integration. Please provide feedback or request additional scenarios if needed.