Conversation
…n tool) - Added MigrationService to re-analyze stories - Added --migrate CLI flag to Analyzer - Ignored coverage reports - Added tests for migration logic
… with voice selection, playback controls, and active text highlighting.
…dicated UI styles, and connectivity service.
…, updated search and story services, and an increased API page size limit
…urce such as YouTube or Reddit
…d backend filtering.
…(Mistral, OpenRouter), YouTube transcripts, and story processing
…terface, models, controller integration, and comprehensive tests.
Migrate/replace mock analysis
📝 WalkthroughWalkthroughThis pull request implements a comprehensive text-to-speech system replacement, introducing Google Cloud TTS integration with backend API proxy, new pagination and filtering controls for the story feed, mobile navigation improvements, a story analysis migration tool, expanded test projects, database schema updates, and supporting configuration and documentation. Changes
Sequence DiagramsequenceDiagram
participant User as User (Web)
participant SpeechUI as Story Reader UI
participant TtsService as TtsService
participant CloudEngine as CloudTtsEngine
participant Backend as Backend API
participant GoogleCloud as Google Cloud TTS
participant Audio as Audio Player
User->>SpeechUI: Select voice & click Play
SpeechUI->>TtsService: speak(text)
TtsService->>TtsService: Load text chunks via TtsProcessor
TtsService->>CloudEngine: speak(chunk, voiceId, rate)
CloudEngine->>Backend: POST /api/tts/synthesize
Backend->>GoogleCloud: Request MP3 audio
GoogleCloud-->>Backend: MP3 bytes
Backend-->>CloudEngine: MP3 response
CloudEngine->>Audio: Create blob URL & play
Audio->>Audio: Play chunk audio
Audio-->>CloudEngine: onended event
CloudEngine->>TtsService: onChunkEnd callback
TtsService->>TtsService: Advance to next chunk
TtsService->>CloudEngine: speak(nextChunk, voiceId, rate)
loop Until all chunks played
CloudEngine->>Backend: POST /api/tts/synthesize
Backend->>GoogleCloud: Request MP3
GoogleCloud-->>Backend: MP3 bytes
Backend-->>CloudEngine: Response
CloudEngine->>Audio: Play next chunk
end
Audio-->>SpeechUI: All chunks played
SpeechUI->>User: Playback complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR merges the development branch into main with significant changes (119 files, 4498 additions, 7041 deletions). Due to the size of the diff, I performed a targeted review of critical security-sensitive files.
Critical Security Findings
I identified several critical security vulnerabilities that must be addressed before merging:
-
🛑 Hardcoded Database Password (
src/Api/appsettings.json, Line 9): The connection string containsPassword=REPLACE_MEwhich could be committed to version control. While the README indicates user secrets should be used, the fallback in appsettings.json creates a security risk if developers forget to configure secrets properly. -
🛑 Missing Input Validation (
src/Api/Controllers/TtsController.cs, Lines 63-88): TheSynthesizeendpoint accepts arbitrary text without length limits or content validation. This could lead to:- Resource exhaustion (DoS) through extremely long text
- Excessive API costs from Google Cloud TTS
- Potential prompt injection if malicious content is synthesized
-
🛑 Overly Permissive CORS in Production (
src/Api/Extensions/ServiceCollectionExtensions.cs, Lines 46-55): IfAllowedOriginsis empty in production, the API will have no CORS policy, making it inaccessible. This is a logic error that will break production functionality. -
SQL Injection Risk (
src/Api/Controllers/StoriesController.cs, Lines 28-30): While Entity Framework Core provides parameterization, the use of.Contains()on user input (@params.SearchTerm) without sanitization could be exploited. EF Core should handle this safely, but explicit validation is recommended. -
Sensitive Error Information Disclosure (
src/Api/Controllers/TtsController.cs, Lines 50, 95, 100): Exception details (ex.Message) are returned to clients, potentially exposing sensitive configuration details, API keys, or internal system information.
Recommendations
Before merging:
- Add input validation and length limits to TTS endpoints
- Implement proper error handling that doesn't expose internal details
- Fix the CORS configuration logic to handle empty AllowedOrigins in production
- Add explicit input sanitization for search functionality
- Consider adding rate limiting to prevent API abuse
Positive Observations
- User secrets are documented and recommended in README
- Security headers are implemented in
Program.cs - HTTPS redirection is enforced
- The AI prompt injection protection in
StoryAnalyzer.cs(Line 64-65) is a good security practice
Due to the diff size limitations, I was unable to review all 119 changed files. I recommend a focused security review of any authentication, authorization, or data validation logic in the remaining files before merging to production.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/Api.IntegrationTests/BaseIntegrationTest.cs (1)
25-48:⚠️ Potential issue | 🟠 Major
WebApplicationFactoryis not disposed, leaking the test server.
factoryis a local variable that implementsIDisposablebut is never stored or disposed. This leaks the in-memory test server and its associated resources across every test class that inheritsBaseIntegrationTest.Store the factory as a field and dispose it in
DisposeAsync.Proposed fix
public abstract class BaseIntegrationTest : IAsyncLifetime { private MsSqlContainer _dbContainer = null!; + private WebApplicationFactory<Program> _factory = null!; protected HttpClient Client { get; private set; } = null!; protected IServiceScope Scope { get; private set; } = null!; protected AppDbContext DbContext { get; private set; } = null!; public async Task InitializeAsync() { _dbContainer = new MsSqlBuilder("mcr.microsoft.com/mssql/server:2022-CU14-ubuntu-22.04") .Build(); await _dbContainer.StartAsync(); - var factory = new WebApplicationFactory<Program>() + _factory = new WebApplicationFactory<Program>() .WithWebHostBuilder(builder => { ... }); - Client = factory.CreateClient(); - Scope = factory.Services.CreateScope(); + Client = _factory.CreateClient(); + Scope = _factory.Services.CreateScope(); ... } public async Task DisposeAsync() { Scope.Dispose(); + _factory.Dispose(); await _dbContainer.StopAsync(); } }src/Web/src/app/services/story.service.ts (2)
33-41:⚠️ Potential issue | 🟠 Major
loadingsignal is never reset on HTTP error.If the HTTP request fails, the
tapcallback won't execute andloadingremainstrueindefinitely. Consider addingfinalize(() => this.loading.set(false))to the pipe, or handling errors withcatchError.Suggested fix
-import { Observable, tap } from 'rxjs'; +import { Observable, tap, finalize } from 'rxjs';return this.http.get<PagedResult<Story>>(this.apiUrl, { params: httpParams }).pipe( tap(result => { this.stories.set(result.items); this.totalCount.set(result.totalCount); const total = result.totalPages || Math.ceil(result.totalCount / (params?.pageSize || 50)); this.totalPages.set(total); - this.loading.set(false); - }) + }), + finalize(() => this.loading.set(false)) );
12-12:⚠️ Potential issue | 🟠 MajorHardcoded
localhostAPI URL affects multiple services and will break in production.The hardcoded URL
'http://localhost:5285/api/stories'at line 12 also appears incloud-tts-engine.ts(line 35:'http://localhost:5285/api/tts'). This pattern has no environment-specific override mechanism in the project. Configure API base URLs using Angular's injectable providers (viaapp.config.ts) or environment-based configuration to ensure this works across different deployment environments.src/Web/src/app/components/story-reader/story-reader.component.ts (1)
129-133:⚠️ Potential issue | 🟡 MinorTypo:
'ELDRIITCH'should be'ELDRITCH'.Double 'I' — same typo flagged in the CSS. Fix in both places for consistency.
Proposed fix
- if (score >= 8) return 'ELDRIITCH'; + if (score >= 8) return 'ELDRITCH';src/Web/src/app/components/story-reader/story-reader.component.html (1)
18-18:⚠️ Potential issue | 🟡 MinorTypo: "TRANSIMITTED" → "TRANSMITTED".
📝 Proposed fix
- <span class="author-tag">TRANSIMITTED BY: {{ story()!.author }}</span> + <span class="author-tag">TRANSMITTED BY: {{ story()!.author }}</span>src/Web/src/app/components/story-feed/story-feed.component.ts (1)
19-49:⚠️ Potential issue | 🟠 MajorMissing cleanup for
fetchSubscription— potential resource leak on component destruction.The component subscribes via
fetchSubscriptioninside theeffect, but never unsubscribes on destroy. Angular will tear down the effect itself, but the last HTTP subscription will remain active. ImplementOnDestroy(or useDestroyRef) to cancel the in-flight request.🛡️ Proposed fix
-import { Component, OnInit, inject, effect } from '@angular/core'; +import { Component, OnInit, OnDestroy, inject, effect } from '@angular/core'; ... -export class StoryFeedComponent implements OnInit { +export class StoryFeedComponent implements OnInit, OnDestroy { ... + ngOnDestroy(): void { + this.fetchSubscription?.unsubscribe(); + }
🤖 Fix all issues with AI agents
In `@src/Analyzer/Services/MigrationService.cs`:
- Around line 27-29: The query in MigrationService where you build mockStories
uses the hardcoded string "MOCK ANALYSIS" which can diverge from the project's
constant; change the filter to use ConfigConstants.MockAnalysisPrefix instead of
the literal so it matches other usages (e.g., the later check that uses
ConfigConstants.MockAnalysisPrefix) — locate the LINQ on _context.Stories in
MigrationService (the method containing var mockStories = await
_context.Stories.Where(...).ToListAsync()) and replace the hardcoded string with
ConfigConstants.MockAnalysisPrefix.
- Around line 76-79: The single call to _context.SaveChangesAsync() at the end
of the re-analysis routine risks losing all progress if it fails; update the
logic in MigrationService.cs so that after each story is successfully
re-analyzed (inside the success branch of the per-story loop) you call
_context.SaveChangesAsync() (or SaveChangesAsync in small batches) and remove
the final bulk save at the end; also wrap each per-story save in a try/catch to
handle transient DB errors (retry or log and continue) so individual successes
persist even if a later save fails.
In `@src/Api/Controllers/TtsController.cs`:
- Around line 13-15: TtsController is missing authentication so unauthenticated
callers can invoke paid TTS endpoints; add protection by annotating the
controller (or its actions) with the [Authorize] attribute (or a specific
policy/role) and ensure the app's authentication schemes are configured (e.g.,
in Program/Startup via AddAuthentication/AddJwtBearer or your chosen provider).
If you cannot enforce auth immediately, add server-side rate limiting/middleware
on the TtsController endpoints (or use an attribute like
[EnableRateLimiting("tts-policy")]) to limit abuse; reference the TtsController
class and its actions when wiring the auth and rate-limiting configuration.
- Around line 92-101: In the Synthesize endpoint's exception handlers (the catch
blocks for InvalidOperationException ex and general Exception ex) remove the
sensitive exception detail from the HTTP response bodies by eliminating the
"details = ex.Message" properties while preserving the existing
_logger.LogWarning(ex, ...) and _logger.LogError(ex, ...) calls; return the same
generic error objects (e.g. { error = "TTS service configuration error" } and {
error = "Speech synthesis failed" }) without including ex.Message so exception
text is only logged, not leaked to clients.
- Around line 45-50: The catch blocks in TtsController (the exception handlers
that currently return StatusCode(500, new { error = "...", details = ex.Message
})) leak internal exception messages to API clients; change both catch handlers
(the one for the TTS configuration error and the generic Exception catch in the
method that "fetches voices") to return a generic error payload (e.g.,
StatusCode(500, new { error = "TTS service error" }) or "Failed to fetch
voices") without including ex.Message, and keep the full exception details only
in the server logs by relying on _logger.LogError(ex, "...") already present.
In `@src/Api/Services/GoogleTtsService.cs`:
- Around line 80-109: SynthesizeAsync currently sends the raw text to Google TTS
without validating the 5,000-byte request limit; update SynthesizeAsync to check
the UTF8 byte length of the text parameter and if it exceeds 5000 bytes either
(A) split the UTF8-encoded text into multiple chunks under 5000 bytes, create
SynthesisInput for each chunk, call client.SynthesizeSpeechAsync for each chunk
and concatenate the resulting response.AudioContent byte arrays before
returning, or (B) implement the Long Audio Synthesis workflow instead (upload
text to GCS and use the long-audio API) and call that flow from SynthesizeAsync;
ensure you still use GetClient(), preserve voice/AudioConfig creation
(VoiceSelectionParams, AudioConfig), log the validation outcome via
_logger.LogError or _logger.LogInformation, and throw a clear exception when
input is too large and no long-audio path is configured.
In `@src/Api/Services/ITtsService.cs`:
- Line 1: The using of Google.Cloud.TextToSpeech.V1 in ITtsService.cs couples
the abstraction to Google Cloud even though ITtsService and the VoiceInfo record
only use standard .NET types; remove the line "using
Google.Cloud.TextToSpeech.V1" from the file so the interface (ITtsService) and
the VoiceInfo record remain provider-agnostic, verify no other references to
Google.Cloud.TextToSpeech.V1 types exist in this file, and run a quick build to
ensure nothing else needs adjusting.
In `@src/Web/src/app/components/shared/pagination/pagination.component.ts`:
- Around line 12-16: The computed() for pages is reading plain `@Input`()
properties (currentPage, totalPages, maxVisiblePages) so it will not update when
parents change; either convert those inputs to writable signals (e.g., change
`@Input`() currentPage to a signal and update call sites to use this.currentPage()
/ this.totalPages() / this.maxVisiblePages()) and keep the computed to read the
signals, or replace the computed pages with a plain getter that reads the
current `@Input`() values (remove computed and implement get pages() { ... }) so
it recalculates on each change-detection cycle; update any handlers/uses that
call pages or read currentPage/currentPage() accordingly (refer to currentPage,
totalPages, maxVisiblePages, and the pages computed/getter in
pagination.component.ts).
In `@src/Web/src/app/components/story-reader/story-reader.component.ts`:
- Around line 97-117: getHighlightedChunk builds raw HTML by concatenating the
incoming chunk and highlighted word without escaping, which can create malformed
HTML or XSS when chunk contains `<`, `>`, `&`; update getHighlightedChunk to
HTML-escape the text segments before composing the string (escape the computed
before, word and after parts) or alternatively build the span using DOM/text
APIs (e.g., create a span with textContent) and return safe HTML; locate
getHighlightedChunk (uses this.tts.currentChunkIndex() and
this.tts.activeCharIndex() and the "word-highlight" span) and ensure the
returned value is safe for binding via [innerHTML] (or switch the template to
render nodes instead of raw HTML) so no unescaped chunk text is inserted.
In `@src/Web/src/app/services/tts/cloud-tts-engine.ts`:
- Around line 34-35: The static apiBase field in CloudTtsEngine is hardcoded to
'http://localhost:5285/api/tts' which breaks non-local deployments; change it to
use the Angular environment or a relative path: replace the value of private
static apiBase with something like environment.apiBase + '/api/tts' (importing
the environment) or simply '/api/tts' as a fallback so requests go to the same
origin, and ensure any code referencing CloudTtsEngine.apiBase continues to work
with the new value.
- Around line 182-192: The stop() method in cloud-tts-engine.ts currently nulls
audio handlers and the audio object but never revokes the blob URL, leaking
object URLs; update stop() (static) to, before dropping this.audio, check if
this.audio.src is a blob URL (starts with "blob:") and call
URL.revokeObjectURL(this.audio.src), then remove handlers
(this.audio.onended/this.audio.onerror = null) and finally set this.audio =
null; keep incrementing currentRequestId and resetting
isCurrentlySpeaking/isCurrentlyPaused as-is. Ensure you reference the static
stop() method and the this.audio.src property when making the change.
In `@src/Web/src/app/services/tts/tts-voice-manager.ts`:
- Around line 27-32: The native voice list is read via this.synth.getVoices()
(mapped into TtsVoice) but getVoices() can return [] initially; modify
initVoices to wait for voices to load by checking this.synth.getVoices() and if
empty attach a one-time 'voiceschanged' listener (or retry with a small timeout)
before mapping into nativeVoices, then proceed to merge with other providers.
Ensure you reference this.synth.getVoices(), the initVoices routine, and the
TtsVoice mapping so the code only builds nativeVoices after voices are
available.
🟡 Minor comments (14)
README.md-22-22 (1)
22-22:⚠️ Potential issue | 🟡 MinorMissing space between
##and the emoji breaks the heading.Most Markdown parsers require a space after
#characters for proper heading rendering.-##🚀 Key Features +## 🚀 Key Featuressrc/Web/src/app/components/story-feed/story-feed.component.css-95-96 (1)
95-96:⚠️ Potential issue | 🟡 MinorDuplicate comment.
/* Range Input Styling */appears on both Line 95 (unchanged) and Line 96 (added). Remove one.docs/GOOGLE_CLOUD_TTS_SETUP.md-17-17 (1)
17-17:⚠️ Potential issue | 🟡 MinorMissing space in heading — won't render as an
h2in most Markdown parsers.
##Step-by-Step Setupneeds a space after##.📝 Proposed fix
-##Step-by-Step Setup +## Step-by-Step Setupsrc/Web/src/app/components/story-feed/story-feed.component.css-143-144 (1)
143-144:⚠️ Potential issue | 🟡 MinorTypo: "veritcally" → "vertically".
📝 Fix
- /* Center veritcally if needed context dependent, but with absolute positioning might not need */ + /* Center vertically if needed; context dependent, but with absolute positioning might not need */src/Shared/Constants/StoryConstants.cs-9-9 (1)
9-9:⚠️ Potential issue | 🟡 Minor
ShareCountsort field is defined but not handled in the controller's sort switch.
StoriesController.GetStories(Line 57–74) does not include a case forStorySortFields.ShareCount, so requests withsortBy=ShareCountsilently fall through to the default (Upvotes). Either add the sort case or defer adding this constant until it's wired up.src/Web/src/app/components/shared/pagination/pagination.component.html-2-3 (1)
2-3:⚠️ Potential issue | 🟡 MinorCondition should be
totalPages > 1to match the comment and expected behavior.The comment says "Only show if multiple pages" but the condition
totalPages > 0will show pagination controls even when there's only a single page.Proposed fix
<!-- Navigation Controls (Only show if multiple pages) --> - <ng-container *ngIf="totalPages > 0"> + <ng-container *ngIf="totalPages > 1">src/Web/src/app/components/story-reader/story-reader-analysis.css-20-22 (1)
20-22:⚠️ Potential issue | 🟡 MinorFix typo in data-level attribute value: "ELDRIITCH" → "ELDRITCH".
This typo appears consistently across multiple files:
src/Web/src/app/components/story-feed/story-feed.component.css:334src/Web/src/app/components/story-feed/story-feed.component.ts:105src/Web/src/app/components/story-reader/story-reader-analysis.css:20src/Web/src/app/components/story-reader/story-reader.component.ts:130Fix all occurrences to use the correct spelling "ELDRITCH".
src/Web/src/app/services/tts/tts-voice-manager.ts-77-82 (1)
77-82:⚠️ Potential issue | 🟡 MinorStale fallback voice name from Edge TTS.
The preferred-voice heuristic still references
JennyNeural, which is a Microsoft Edge voice name. The class docstring (Line 8) says this now uses Google Cloud TTS. Consider updating the fallback list to reference actual Google Cloud voice names for consistency, or at least document that this is intentional for native-voice fallback.src/Api/Services/GoogleTtsService.cs-25-48 (1)
25-48:⚠️ Potential issue | 🟡 Minor
GetClient()is not thread-safe — concurrent callers can initialize multiple clients.If
GoogleTtsServiceis registered as a singleton (typical for DI-managed services), multiple concurrent requests could both see_ttsClient == nulland build separate clients. While the race is benign (no data corruption), it wastes resources and is easy to fix.🔒 Proposed fix: use `Lazy` for thread-safe lazy initialization
public class GoogleTtsService : ITtsService { private readonly ILogger<GoogleTtsService> _logger; private readonly IConfiguration _configuration; - private TextToSpeechClient? _ttsClient; + private readonly Lazy<TextToSpeechClient> _ttsClient; public GoogleTtsService(ILogger<GoogleTtsService> logger, IConfiguration configuration) { _logger = logger; _configuration = configuration; + _ttsClient = new Lazy<TextToSpeechClient>(CreateClient); } - private TextToSpeechClient GetClient() + private TextToSpeechClient CreateClient() { - if (_ttsClient != null) return _ttsClient; - var apiKey = _configuration["GoogleCloud:TtsApiKey"]; if (string.IsNullOrEmpty(apiKey)) { _logger.LogWarning("Google Cloud TTS API key not configured"); throw new InvalidOperationException( "Google Cloud TTS API key not found. " + "Please add 'GoogleCloud:TtsApiKey' to user secrets or appsettings.json"); } var clientBuilder = new TextToSpeechClientBuilder { ApiKey = apiKey }; - _ttsClient = clientBuilder.Build(); + var client = clientBuilder.Build(); _logger.LogInformation("Google Cloud TTS client initialized"); - - return _ttsClient; + return client; }Then replace
GetClient()calls with_ttsClient.Value.docs/TTS_INTEGRATION_SUMMARY.md-272-279 (1)
272-279:⚠️ Potential issue | 🟡 MinorExample output resembles a real API key prefix.
Line 278 shows
AIzaSy...which matches the Google API key prefix pattern. While it's clearly placeholder text, consider using a more obviously fake value (e.g.,YOUR_API_KEY_HERE) to avoid any confusion during secret scanning or code audits.src/Web/src/app/services/tts/cloud-tts-engine.ts-207-212 (1)
207-212:⚠️ Potential issue | 🟡 MinorUnhandled promise from
audio.play()inresume().
HTMLMediaElement.play()returns aPromisethat rejects if autoplay is blocked. Ignoring it triggers an unhandled promise rejection.🛡️ Proposed fix
static resume() { if (this.audio && this.audio.paused) { - this.audio.play(); + this.audio.play().catch(err => { + console.error('[CloudTTS] Resume playback failed:', err); + this.isCurrentlyPaused = true; + }); this.isCurrentlyPaused = false; } }docs/TTS_INTEGRATION_SUMMARY.md-61-67 (1)
61-67:⚠️ Potential issue | 🟡 MinorAdd language specifiers to fenced code blocks.
Static analysis (markdownlint MD040) flags code blocks at lines 61, 79, and 295 as missing a language specifier. Adding
textor an appropriate language tag improves rendering and accessibility.Example fix for line 61
-``` +```text Frontend (Angular)Apply the same pattern to the other two untagged blocks (~line 79, ~line 295).
src/Web/src/app/services/tts.service.ts-531-534 (1)
531-534:⚠️ Potential issue | 🟡 Minor
loadPreferencesdoesn't validate the parsed rate —NaNcould propagate.If
localStoragecontains a non-numeric string fortts_rate,parseFloatreturnsNaN, which would break rate calculations (e.g.,estimatedDuration). Add a guard.🛡️ Proposed fix
private loadPreferences() { const savedRate = localStorage.getItem('tts_rate'); - if (savedRate) this.rate.set(parseFloat(savedRate)); + if (savedRate) { + const parsed = parseFloat(savedRate); + if (!isNaN(parsed) && parsed > 0) this.rate.set(parsed); + } }src/Web/src/app/components/story-feed/story-feed.component.ts-99-102 (1)
99-102:⚠️ Potential issue | 🟡 MinorPage number not reset when page size changes.
When
pageSizechanges, the current page may exceed the newtotalPages(e.g., page 5 of 3). Resetpageto 1 alongside the size change to avoid requesting an out-of-range page.🐛 Proposed fix
onPageSizeChange(size: number) { this.searchService.setPageSize(size); + this.searchService.setPage(1); window.scrollTo({ top: 0, behavior: 'smooth' }); }
🧹 Nitpick comments (41)
src/Web/src/app/services/connectivity.service.ts (1)
10-10: Consider exposingisOnlineas a read-only signal.
signal()returns aWritableSignal, so any consumer that injects this service can callisOnline.set(…)and corrupt the state. Exposing a read-only view prevents accidental external mutations.♻️ Suggested change
+import { Injectable, signal, OnDestroy, Signal } from '@angular/core'; -import { Injectable, signal, OnDestroy } from '@angular/core'; `@Injectable`({ providedIn: 'root' }) export class ConnectivityService implements OnDestroy { - public isOnline = signal<boolean>(navigator.onLine); + private readonly _isOnline = signal<boolean>(navigator.onLine); + public readonly isOnline: Signal<boolean> = this._isOnline.asReadonly(); - private onlineHandler = () => this.isOnline.set(true); - private offlineHandler = () => this.isOnline.set(false); + private onlineHandler = () => this._isOnline.set(true); + private offlineHandler = () => this._isOnline.set(false);src/Crawler.Tests/RedditServiceTests.cs (2)
14-19: Unused test constants.
TestId,TestTitle,TestAuthor,TestPermalink,TestBody, andTestUpsare declared but never referenced. The JSON payloads and assertions throughout this file still use hard-coded literals. Either wire these constants into the JSON templates and assertions, or remove them to avoid confusion.
149-155: Strengthen theExternalIdassertion to verify it's a valid GUID.
Assert.NotNullwould pass even if the fallback were an empty string or arbitrary text. Since the comment states a GUID should be generated, validate the format:Suggested improvement
- Assert.NotNull(result[0].ExternalId); // Should generate a Guid if id is null + Assert.True(Guid.TryParse(result[0].ExternalId, out _), "ExternalId should be a valid GUID when id is null");.gitignore (1)
485-487: LGTM! Consider co-locating with other coverage patterns.The ignore pattern is correct and will properly exclude the coverage report directory from version control, aligning well with the cleanup of previously committed coverage artifacts.
For improved maintainability, consider moving this pattern near the other coverage-related entries (lines 144-158) where DotCover, AxoCover, and Coverlet patterns are grouped together.
📁 Optional refactor: Co-locate coverage patterns
Move the new pattern to line 159 (right after the existing coverage patterns) instead of the current location:
# Visual Studio code coverage results *.coverage *.coveragexml + +# Code Coverage Report +coveragereport/ # NCrunchThen remove it from the current location at the end of the file.
src/Crawler.Tests/StoryProcessorTests.cs (1)
79-98: Good test coverage for event production.The test correctly verifies that
Produceis called with the expectedStoryFetchedevent. One minor note: the_mockProduceris never explicitly set up to returnTask.CompletedTaskforProduce. Modern Moq versions handle this by default, but an explicit setup makes the test more resilient and self-documenting.Suggested setup (add to constructor or Arrange block)
_mockProducer = new Mock<ITopicProducer<StoryFetched>>(); +_mockProducer + .Setup(p => p.Produce(It.IsAny<StoryFetched>(), It.IsAny<CancellationToken>())) + .Returns(Task.CompletedTask);src/Analyzer.Tests/StoryAnalyzerTests.cs (1)
221-238: Minor inconsistency: higher-priority providers not explicitly disabled.This test relies on Moq returning
nullfor un-setup config keys to skip Gemini and DeepSeek. It works, but the OpenRouter test (Line 244-248) explicitly nulls all higher-priority providers. Consider matching that pattern here for clarity and resilience against future refactors (e.g., switching toConfigurationBuilderwith defaults).🔧 Suggested: explicitly null higher-priority providers for consistency
// Arrange + _config.Setup(c => c["GEMINI_API_KEY"]).Returns((string?)null); + _config.Setup(c => c["DEEPSEEK_API_KEY"]).Returns((string?)null); _config.Setup(c => c["MISTRAL_API_KEY"]).Returns("fake_mistral_key");src/Analyzer.Tests/MigrationServiceTests.cs (1)
11-29: Consider disposingAppDbContextafter each test.
AppDbContextimplementsIDisposable. While in-memory databases are lightweight, implementingIDisposableon the test class ensures proper cleanup.♻️ Suggested fix
-public class MigrationServiceTests +public class MigrationServiceTests : IDisposable { private readonly AppDbContext _context; private readonly Mock<IStoryAnalyzer> _mockAnalyzer; private readonly Mock<ILogger<MigrationService>> _mockLogger; private readonly MigrationService _service; public MigrationServiceTests() { // ... existing code ... } + + public void Dispose() + { + _context.Dispose(); + }src/Analyzer/Services/MigrationService.cs (1)
37-37: Use structured logging templates instead of string interpolation.All
LogInformation/LogWarning/LogErrorcalls use$"..."interpolation, which defeats structured logging (no semantic parameters for log aggregation) and allocates strings even when the log level is filtered out. Use message templates with named placeholders instead.Example:
-_logger.LogInformation($"📝 Found {mockStories.Count} stories to migrate."); +_logger.LogInformation("📝 Found {StoryCount} stories to migrate.", mockStories.Count);Also applies to: 44-44, 55-55, 64-64, 70-70, 82-84
src/Api.Tests/UnitTest1.cs (1)
1-9: Remove scaffold placeholder test.This is an auto-generated boilerplate from project creation. Since the PR already includes real tests (
TtsControllerTests,GoogleTtsServiceTests), this empty test adds noise and should be removed.src/Web/src/app/components/story-reader/styles/tts/tts-animations.css (1)
14-23:slideInUpduplicatesfadeInfromstyles.css.
slideInUp(translateY 10px → 0 + opacity) is identical to the globalfadeInkeyframe instyles.css(lines 77-87). Consider reusing the existing animation or extracting a shared utility, though having a TTS-scoped copy is acceptable if you prefer module isolation.src/Api.IntegrationTests/BaseIntegrationTest.cs (1)
55-59:DisposeAsyncshould also dispose theClient.
HttpClientisIDisposable. WhileWebApplicationFactory.CreateClient()creates the client, disposing the factory alone may not deterministically clean up all HTTP handler resources. Explicitly disposing the client before the factory is good hygiene.public async Task DisposeAsync() { + Client.Dispose(); Scope.Dispose(); await _dbContainer.StopAsync(); }README.md (1)
120-128: Section numbering is inconsistent — "5. Maintenance" follows "4. Run the Abyss" but the Testing section sits between them.The "Getting Started" subsections jump from 2 → 4 (step 3 is missing), and this new section 5 appears after an unrelated "Testing & Code Coverage" heading. Consider renumbering or restructuring so the maintenance section is either part of "Getting Started" with correct numbering or a standalone top-level section.
src/Shared.Tests/EnvLoaderTests.cs (1)
6-70: Environment variables set during tests are never cleaned up.Each test sets process-wide environment variables (via
EnvLoader.LoadorEnvironment.SetEnvironmentVariable) butDisposeonly deletes the file. While GUIDs prevent collisions, the variables persist for the lifetime of the test process.Consider clearing them in
Dispose:Sketch
Track the keys and clean up:
public class EnvLoaderTests : IDisposable { private readonly string _testEnvFile = "test.env"; + private readonly List<string> _envKeysToClean = new(); ... + private void TrackKey(string key) => _envKeysToClean.Add(key); public void Dispose() { if (File.Exists(_testEnvFile)) File.Delete(_testEnvFile); + foreach (var key in _envKeysToClean) + Environment.SetEnvironmentVariable(key, null); } }Then call
TrackKey(key)after eachvar key = ...assignment.src/Api/Models/TtsModels.cs (1)
6-18: Consider adding validation attributes forTextandRate.The XML comments document constraints (max ~5000 chars for
Text, 0.25–4.0 forRate) but nothing enforces them. Unbounded text could hit Google API limits or inflate costs. Data annotations would catch this at the controller level automatically.♻️ Suggested addition
+using System.ComponentModel.DataAnnotations; + namespace Api.Models; public record SynthesizeRequest { - /// <summary>Text to synthesize (max ~5000 characters)</summary> - public required string Text { get; init; } + /// <summary>Text to synthesize (max 5000 characters)</summary> + [Required, MaxLength(5000)] + public required string Text { get; init; } /// <summary>Voice ID (e.g., "en-US-Neural2-C")</summary> public required string VoiceId { get; init; } /// <summary>Language code (default: "en-US")</summary> public string? LanguageCode { get; init; } - /// <summary>Speaking rate (0.25 to 4.0, default: 1.0)</summary> - public double Rate { get; init; } = 1.0; + /// <summary>Speaking rate (0.25 to 4.0, default: 1.0)</summary> + [Range(0.25, 4.0)] + public double Rate { get; init; } = 1.0; }src/Web/src/app/services/tts/tts-engine.ts (1)
1-2:window.speechSynthesisaccessed eagerly — will crash in non-browser contexts.The static property initializer runs at import time. If this code is ever loaded during SSR, tests, or in a browser that doesn't support the Web Speech API, it will throw. Consider lazy initialization with a guard.
♻️ Suggested defensive approach
export class TtsEngine { - private static synth = window.speechSynthesis; + private static get synth(): SpeechSynthesis { + if (typeof window === 'undefined' || !window.speechSynthesis) { + throw new Error('SpeechSynthesis API is not available'); + } + return window.speechSynthesis; + }src/Api/Extensions/ServiceCollectionExtensions.cs (1)
59-60: ConsiderSingletonlifetime forGoogleTtsService.
GoogleTtsServicelazily creates aTextToSpeechClient(gRPC channel), butAddScopedmeans a new instance—and new gRPC client—is created per HTTP request. gRPC clients are designed to be long-lived and reused. Registering asSingleton(or extracting the client into a singleton) would avoid repeated channel setup overhead.src/Api/Controllers/StoriesController.cs (1)
43-54: Platform filter: useOrdinalIgnoreCaseand consider extensibility.
ToLower()is culture-sensitive. UseStringComparison.OrdinalIgnoreCaseorToLowerInvariant()instead.- Unrecognized platform values are silently ignored — consider returning a 400 or logging a warning so API consumers know their filter had no effect.
Suggested fix for culture-safe comparison
- if (!string.IsNullOrWhiteSpace(`@params.Platform`)) - { - var platform = `@params.Platform.ToLower`(); - if (platform == "youtube") - { - query = query.Where(s => s.Url.Contains("youtube.com") || s.Url.Contains("youtu.be")); - } - else if (platform == "reddit") - { - query = query.Where(s => s.Url.Contains("reddit.com")); - } - } + if (!string.IsNullOrWhiteSpace(`@params.Platform`)) + { + if (string.Equals(`@params.Platform`, "youtube", StringComparison.OrdinalIgnoreCase)) + { + query = query.Where(s => s.Url.Contains("youtube.com") || s.Url.Contains("youtu.be")); + } + else if (string.Equals(`@params.Platform`, "reddit", StringComparison.OrdinalIgnoreCase)) + { + query = query.Where(s => s.Url.Contains("reddit.com")); + } + }src/Web/src/app/components/shared/pagination/pagination.component.css (2)
96-100: Disabled button contrast may not meet WCAG AA requirements.
opacity: 0.3on disabled navigation buttons could push the text below the 4.5:1 contrast ratio threshold against a dark background. Consider usingopacity: 0.4–0.5or explicitly setting a higher-contrast disabled color while still conveying the disabled state visually.
107-113: Duplicate styles for.next .nav-iconand.prev .nav-icon.Both rules are identical. Combine them.
Suggested fix
-.next .nav-icon { - margin-top: -2px; -} - -.prev .nav-icon { - margin-top: -2px; -} +.next .nav-icon, +.prev .nav-icon { + margin-top: -2px; +}src/Web/src/app/components/story-reader/styles/tts/tts-highlight.css (1)
26-32: Scope::ng-deepwith:hostto prevent style leakage.
::ng-deepwithout a:hostprefix makes this rule globally applied to the entire DOM, not just this component's subtree. This can unintentionally style.word-highlightelements in other components.Note: The Biome lint error about
::ng-deepbeing an unknown pseudo-element is a false positive — it's an Angular-specific construct. However,::ng-deepis deprecated in Angular (though still functional). Consider using global styles or:host ::ng-deepas a scoping measure.♻️ Proposed fix to scope the deep style
-::ng-deep .word-highlight { +:host ::ng-deep .word-highlight { background: rgba(var(--primary-glow-rgb, 98, 0, 234), 0.5); color: `#fff`; border-radius: 2px; box-shadow: 0 0 10px rgba(var(--primary-glow-rgb, 98, 0, 234), 0.3); padding: 0 2px; }src/Api/Models/StoryQueryParameters.cs (1)
14-20: Consider cross-validatingMinScaryScore≤MaxScaryScoreand constrainingPlatform.Two observations:
- When both
MinScaryScoreandMaxScaryScoreare provided, there's no validation thatMin ≤ Max. This silently returns empty results rather than signaling a client error.Platformis an unconstrained string. If only a known set of values is expected (e.g., "Reddit", "YouTube"), consider validating with an allow-list or enum to reject typos/invalid values early.Neither is critical since these are read-only filters, but both would improve API robustness.
src/Web/src/app/components/shared/pagination/pagination.component.spec.ts (2)
28-34: Loose assertion — consider verifying the full pages array.The comment documents the expected output as
[1, 2, 3, 4, 5, '...', 10], but the assertions only check that1,'...', and10are present. This would pass even if the pagination logic produced[1, '...', 10]or[10, 1, '...', 5, 3]. Consider usingtoEqualagainst the full expected array to catch regressions in the pagination algorithm.♻️ Proposed stricter assertion
it('should calculate pages correctly for page 1', () => { const pages = component.pages(); - // Expected: [1, 2, 3, 4, 5, '...', 10] - expect(pages).toContain(1); - expect(pages).toContain('...'); - expect(pages).toContain(10); + expect(pages).toEqual([1, 2, 3, 4, 5, '...', 10]); });
48-61: Missing boundary tests foronPrevon first page andonNexton last page.The suite covers
onNextfrom page 1 andonPrevfrom page 2, but doesn't test the guard conditions: callingonPrevwhen already on page 1 andonNextwhen on the last page. These are important boundary cases to ensure the component doesn't emit invalid page numbers.♻️ Suggested additional tests
it('should not emit pageChange on onPrev when on first page', () => { const spy = vi.spyOn(component.pageChange, 'emit'); component.onPrev(); expect(spy).not.toHaveBeenCalled(); }); it('should not emit pageChange on onNext when on last page', () => { fixture.componentRef.setInput('currentPage', 10); fixture.detectChanges(); const spy = vi.spyOn(component.pageChange, 'emit'); component.onNext(); expect(spy).not.toHaveBeenCalled(); });src/Web/src/app/components/story-reader/story-reader-analysis.css (1)
90-117:::ng-deepis deprecated in Angular — consider alternatives.The Biome linter flags these as unknown pseudo-elements, which is a false positive (it's Angular-specific). However,
::ng-deephas been deprecated by the Angular team. It still works but may be removed in a future version. Consider using:host ::ng-deepif you must pierce encapsulation, or restructure to use global styles orViewEncapsulation.Nonefor this component's analysis text styles.src/Web/src/app/components/story-reader/story-reader.component.ts (1)
33-41: Unused variableindexin effect — clarify intent with a comment or void expression.
indexis declared solely to subscribe to thecurrentChunkIndexsignal but is never used. This reads as a mistake. Use a void expression or add a brief comment to signal intent.Proposed fix
effect(() => { - const index = this.tts.currentChunkIndex(); - // Only auto-scroll if playing to avoid annoying jumps when just exploring + // Subscribe to chunk index changes to trigger auto-scroll + void this.tts.currentChunkIndex(); if (this.tts.isPlaying() && !this.tts.isPaused()) { setTimeout(() => this.scrollToActiveChunk(), 100); } });src/Api.IntegrationTests/TtsControllerIntegrationTests.cs (1)
25-33: Mock registered asScopedbut the instance is effectively a singleton.
ServiceDescriptor.Scoped<ITtsService>(_ => _ttsServiceMock.Object)returns the same mock object for every scope. This works for the current tests but is misleading — consider usingServiceDescriptor.Singletonto match the actual lifetime, or at minimum add a brief comment clarifying the intent.src/Api/Controllers/TtsController.cs (1)
63-76: Consider adding a max text length validation.
request.Textis passed directly to Google Cloud TTS with only an empty-check. An arbitrarily large payload could lead to excessive costs or hit Google's limits ungracefully. A simple length guard (e.g., 5000 chars) would provide a cleaner experience.Proposed fix
if (string.IsNullOrWhiteSpace(request.Text)) { return BadRequest(new { error = "Text is required" }); } + + if (request.Text.Length > 5000) + { + return BadRequest(new { error = "Text exceeds maximum length of 5000 characters" }); + }src/Web/src/app/components/story-feed/story-feed.component.html (2)
127-129: Misleading indentation on closing tags — nesting appears broken at a glance.Lines 127-129 close
card-footer,card-content, andarticlerespectively, but all three sit at the same indentation level. While the nesting is technically correct, this makes it very difficult to visually verify correctness during future edits.Proposed fix
- </div> - </div> - </article> + </div> <!-- card-footer --> + </div> <!-- card-content --> + </article>
86-97:getStorySource(story.url)called three times per card.This method is invoked on lines 86, 87, and 97 for each story. In Angular templates, method calls execute on every change detection cycle. Consider computing this once — either as a property on the story model or via a local variable with
@let/aspattern — to avoid redundant work, especially with many cards.src/Web/src/app/app.css (2)
29-30: Inconsistent indentation onletter-spacing.Line 30 uses deeper indentation (8 spaces) compared to the sibling
font-sizeproperty on line 29 (4 spaces).Proposed fix
.logo-text { font-size: 1.1rem; - letter-spacing: 0.25em; + letter-spacing: 0.25em; color: var(--accent-silver);
269-295: Inconsistent indentation throughout the 1024px media query block.The
.sidebar,.sidebar.open,.sidebar-backdrop, and.top-headerrules inside this media query use a mix of indentation depths (4, 8, and 12 spaces), making it hard to tell which rules are at the same nesting level. They should all be indented one level inside@media.Proposed fix (consistent 4-space indent inside media query)
`@media` (max-width: 1024px) { .menu-toggle { display: block; } - .sidebar { - transform: translateX(-100%); - transition: transform 0.4s cubic-bezier(0.4, 0, 0.2, 1); - z-index: 1000; - box-shadow: 20px 0 50px rgba(0, 0, 0, 0.5); - } - - .sidebar.open { - transform: translateX(0); - } - - .sidebar-backdrop { - display: block; + .sidebar { + transform: translateX(-100%); + transition: transform 0.4s cubic-bezier(0.4, 0, 0.2, 1); + z-index: 1000; + box-shadow: 20px 0 50px rgba(0, 0, 0, 0.5); + } + + .sidebar.open { + transform: translateX(0); + } + + .sidebar-backdrop { + display: block; } .main-content { margin-left: 0; } -.top-header { - margin: 1rem; - padding: 0 1.5rem; -} + + .top-header { + margin: 1rem; + padding: 0 1.5rem; + } }src/Web/src/app/services/tts/tts-voice-manager.ts (1)
41-48:as any[]cast discards type safety on cloud voice response.Consider defining a lightweight response interface (e.g.,
{ name: string; voiceId: string; languageCode: string }) to type therawCloudVoicesinstead of casting toany[]. This would catch mapping errors at compile time.src/Api.Tests/Controllers/TtsControllerTests.cs (1)
79-90: Consider adding a test for the Synthesize error (500) path.There is a test for the
GetVoicesexception path but no corresponding test whenSynthesizeAsyncthrows. Adding aSynthesize_ReturnsInternalServerError_WhenExceptionOccurstest would ensure parity in error-handling coverage.src/Web/src/app/components/story-reader/story-reader.component.html (3)
110-126: Inconsistent indentation makes nesting hard to follow.The closing tags from lines 119–126 (
</div>,</div>,</ng-container>,</div>) have mismatched indentation that doesn't reflect the actual DOM nesting. While not a functional bug, this makes the template fragile to edit. Consider reformatting to match the nesting depth.
153-180: Duplicated playback controls between main toolbar and floating mini-player.The play/pause/resume/stop button logic on lines 156–178 is largely duplicated from the main toolbar (lines 96–125). Consider extracting the shared playback controls into a small reusable component (e.g.,
<app-tts-controls>) to reduce duplication and keep behavior in sync.
131-140: UseDomSanitizerexplicitly ingetHighlightedChunkand returnSafeHtml.Line 134 uses
[innerHTML]="getHighlightedChunk(chunk, i)"to render highlighted text. While the method constructs HTML by concatenating strings with a word-highlight span (line 113), Angular's[innerHTML]binding provides automatic sanitization by default. However, the component already injectsDomSanitizer(line 29)—explicitly sanitize the returned HTML ingetHighlightedChunkusingthis.sanitizer.sanitize(SecurityContext.HTML, ...)or return aSafeHtmlobject viabypassSecurityTrustHtml()to follow Angular security best practices and make the sanitization intent explicit.src/Web/src/app/components/story-feed/story-feed.component.ts (1)
52-54:OnInitis declared but the body is empty.Since the initial fetch is driven by the constructor
effect, theOnInitinterface and emptyngOnInitare unused boilerplate. Consider removing them to keep the class surface clean.src/Web/src/app/services/tts/cloud-tts-engine.ts (1)
231-231: Return typeany[]loses type safety.
getVoices()returnsPromise<any[]>. Consider defining or reusing a typed interface (e.g., the backend'sVoiceInfoshape) to get compile-time checks on consumers likeTtsVoiceManager.src/Web/src/app/services/tts.service.ts (2)
387-422: Seek debounce viasetTimeoutdoesn't cancel the previous timer.If the user drags the seek bar rapidly, each call queues a new 200ms
setTimeoutwithout clearing the previous one. MultipleplayChunkcalls can fire in quick succession, all capturing the samecurrentSessionId. While the engines' internal cancellation mechanisms prevent audible overlap in most cases, storing and clearing the timer handle would make the intent explicit and bulletproof.♻️ Proposed fix
Add a private field and clear it on each seek:
+ private seekTimeout: ReturnType<typeof setTimeout> | null = null; ... seek(percentage: number, forcePlay = false) { if (this.chunks().length === 0) return; this.currentSessionId++; this.activeCharIndex.set(-1); TtsEngine.stop(); CloudTtsEngine.stop(); + if (this.seekTimeout) { + clearTimeout(this.seekTimeout); + this.seekTimeout = null; + } ... if (forcePlay || (this.isPlaying() && !this.isPaused())) { this.isPaused.set(false); this.isPlaying.set(true); - setTimeout(() => this.playChunk(targetIndex), 200); + this.seekTimeout = setTimeout(() => this.playChunk(targetIndex), 200); } else {
536-541: Persistence effect eagerly writes on construction and couples two independent concerns.This effect fires immediately (writing default rate + null voice to storage) and re-fires whenever either signal changes, writing both values even when only one changed. Consider splitting into two effects or using explicit save calls triggered by
setVoice/setRateto avoid unnecessary writes.src/Crawler.Tests/YouTubeServiceTests.cs (1)
85-121: Well-structured test covering the transcript happy path.The mock chain (search → video → manifest → track) correctly mirrors the production code in
GetStoriesFromChannelAsync, and the assertions properly verify that caption text lands inBodyText.Minor nit: Line 101 has a duplicate
// Mock Manifestcomment.🧹 Remove duplicate comment
- // Mock Manifest // Mock Manifest,
| var mockStories = await _context.Stories | ||
| .Where(s => s.AiAnalysis.Contains("MOCK ANALYSIS")) | ||
| .ToListAsync(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use ConfigConstants.MockAnalysisPrefix instead of the hardcoded string.
Line 28 uses the hardcoded "MOCK ANALYSIS" while Line 53 uses ConfigConstants.MockAnalysisPrefix ("MOCK ANALYSIS:"). Use the constant consistently to avoid subtle mismatches if the prefix ever changes.
♻️ Proposed fix
var mockStories = await _context.Stories
- .Where(s => s.AiAnalysis.Contains("MOCK ANALYSIS"))
+ .Where(s => s.AiAnalysis.Contains(ConfigConstants.MockAnalysisPrefix))
.ToListAsync();🤖 Prompt for AI Agents
In `@src/Analyzer/Services/MigrationService.cs` around lines 27 - 29, The query in
MigrationService where you build mockStories uses the hardcoded string "MOCK
ANALYSIS" which can diverge from the project's constant; change the filter to
use ConfigConstants.MockAnalysisPrefix instead of the literal so it matches
other usages (e.g., the later check that uses
ConfigConstants.MockAnalysisPrefix) — locate the LINQ on _context.Stories in
MigrationService (the method containing var mockStories = await
_context.Stories.Where(...).ToListAsync()) and replace the hardcoded string with
ConfigConstants.MockAnalysisPrefix.
| } | ||
|
|
||
| // 4. Save Changes | ||
| await _context.SaveChangesAsync(); |
There was a problem hiding this comment.
Single SaveChangesAsync at the end risks losing all progress if it fails.
If SaveChangesAsync throws (e.g., transient DB error), all successfully re-analyzed stories are lost and would need to be re-processed. Consider saving in batches or after each successful update, especially since each iteration already incurs a 1-second delay.
♻️ Option: save per-story inside the success branch
else
{
story.AiAnalysis = result.Analysis;
story.ScaryScore = result.Score;
+ await _context.SaveChangesAsync();
_logger.LogInformation($"✅ Successfully re-analyzed '{story.Title}'. Score: {story.ScaryScore}");
successCount++;
}Then remove the bulk save at Line 79.
🤖 Prompt for AI Agents
In `@src/Analyzer/Services/MigrationService.cs` around lines 76 - 79, The single
call to _context.SaveChangesAsync() at the end of the re-analysis routine risks
losing all progress if it fails; update the logic in MigrationService.cs so that
after each story is successfully re-analyzed (inside the success branch of the
per-story loop) you call _context.SaveChangesAsync() (or SaveChangesAsync in
small batches) and remove the final bulk save at the end; also wrap each
per-story save in a try/catch to handle transient DB errors (retry or log and
continue) so individual successes persist even if a later save fails.
| [ApiController] | ||
| [Route("api/[controller]")] | ||
| public class TtsController : ControllerBase |
There was a problem hiding this comment.
No authentication on a paid API proxy.
The controller doc says "secure backend proxy," but there are no [Authorize] attributes. Any unauthenticated caller can invoke Google Cloud TTS through these endpoints, which incurs costs. Consider adding authentication/authorization or at minimum rate-limiting.
🤖 Prompt for AI Agents
In `@src/Api/Controllers/TtsController.cs` around lines 13 - 15, TtsController is
missing authentication so unauthenticated callers can invoke paid TTS endpoints;
add protection by annotating the controller (or its actions) with the
[Authorize] attribute (or a specific policy/role) and ensure the app's
authentication schemes are configured (e.g., in Program/Startup via
AddAuthentication/AddJwtBearer or your chosen provider). If you cannot enforce
auth immediately, add server-side rate limiting/middleware on the TtsController
endpoints (or use an attribute like [EnableRateLimiting("tts-policy")]) to limit
abuse; reference the TtsController class and its actions when wiring the auth
and rate-limiting configuration.
| return StatusCode(500, new { error = "TTS service configuration error", details = ex.Message }); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Failed to fetch voices"); | ||
| return StatusCode(500, new { error = "Failed to fetch voices", details = ex.Message }); |
There was a problem hiding this comment.
Exception details leaked to API consumers.
Both catch blocks return ex.Message in the response body. This exposes internal implementation details (e.g., "Google Cloud TTS API key not found", connection strings, or Google API errors) to external callers. Return a generic error message to the client and keep the details in server logs only.
Proposed fix
catch (InvalidOperationException ex)
{
_logger.LogWarning(ex, "TTS configuration issue");
- return StatusCode(500, new { error = "TTS service configuration error", details = ex.Message });
+ return StatusCode(500, new { error = "TTS service configuration error" });
}
catch (Exception ex)
{
_logger.LogError(ex, "Failed to fetch voices");
- return StatusCode(500, new { error = "Failed to fetch voices", details = ex.Message });
+ return StatusCode(500, new { error = "Failed to fetch voices" });
}🤖 Prompt for AI Agents
In `@src/Api/Controllers/TtsController.cs` around lines 45 - 50, The catch blocks
in TtsController (the exception handlers that currently return StatusCode(500,
new { error = "...", details = ex.Message })) leak internal exception messages
to API clients; change both catch handlers (the one for the TTS configuration
error and the generic Exception catch in the method that "fetches voices") to
return a generic error payload (e.g., StatusCode(500, new { error = "TTS service
error" }) or "Failed to fetch voices") without including ex.Message, and keep
the full exception details only in the server logs by relying on
_logger.LogError(ex, "...") already present.
| catch (InvalidOperationException ex) | ||
| { | ||
| _logger.LogWarning(ex, "TTS configuration issue during synthesis"); | ||
| return StatusCode(500, new { error = "TTS service configuration error", details = ex.Message }); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Failed to synthesize speech"); | ||
| return StatusCode(500, new { error = "Speech synthesis failed", details = ex.Message }); | ||
| } |
There was a problem hiding this comment.
Same exception detail leakage in the Synthesize endpoint.
Same issue as GetVoices — remove details = ex.Message from both catch blocks here as well.
Proposed fix
catch (InvalidOperationException ex)
{
_logger.LogWarning(ex, "TTS configuration issue during synthesis");
- return StatusCode(500, new { error = "TTS service configuration error", details = ex.Message });
+ return StatusCode(500, new { error = "TTS service configuration error" });
}
catch (Exception ex)
{
_logger.LogError(ex, "Failed to synthesize speech");
- return StatusCode(500, new { error = "Speech synthesis failed", details = ex.Message });
+ return StatusCode(500, new { error = "Speech synthesis failed" });
}📝 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.
| catch (InvalidOperationException ex) | |
| { | |
| _logger.LogWarning(ex, "TTS configuration issue during synthesis"); | |
| return StatusCode(500, new { error = "TTS service configuration error", details = ex.Message }); | |
| } | |
| catch (Exception ex) | |
| { | |
| _logger.LogError(ex, "Failed to synthesize speech"); | |
| return StatusCode(500, new { error = "Speech synthesis failed", details = ex.Message }); | |
| } | |
| catch (InvalidOperationException ex) | |
| { | |
| _logger.LogWarning(ex, "TTS configuration issue during synthesis"); | |
| return StatusCode(500, new { error = "TTS service configuration error" }); | |
| } | |
| catch (Exception ex) | |
| { | |
| _logger.LogError(ex, "Failed to synthesize speech"); | |
| return StatusCode(500, new { error = "Speech synthesis failed" }); | |
| } |
🤖 Prompt for AI Agents
In `@src/Api/Controllers/TtsController.cs` around lines 92 - 101, In the
Synthesize endpoint's exception handlers (the catch blocks for
InvalidOperationException ex and general Exception ex) remove the sensitive
exception detail from the HTTP response bodies by eliminating the "details =
ex.Message" properties while preserving the existing _logger.LogWarning(ex, ...)
and _logger.LogError(ex, ...) calls; return the same generic error objects (e.g.
{ error = "TTS service configuration error" } and { error = "Speech synthesis
failed" }) without including ex.Message so exception text is only logged, not
leaked to clients.
| @Input({ required: true }) currentPage!: number; | ||
| @Input({ required: true }) totalPages!: number; | ||
| @Input() maxVisiblePages = 5; | ||
| @Input() pageSize = 50; | ||
| @Input() pageSizeOptions = [10, 20, 50, 100, 200]; |
There was a problem hiding this comment.
Critical: computed() won't react to @Input() property changes.
computed() only tracks Angular signal reads. The properties currentPage, totalPages, and maxVisiblePages are declared with @Input() (non-signal), so pages will be computed once at construction time and never update when parent binds new values.
Either convert to signal-based inputs or use a getter instead of computed().
🐛 Option 1 (preferred): Use signal inputs
-import { Component, Input, Output, EventEmitter, computed } from '@angular/core';
+import { Component, input, Output, EventEmitter, computed } from '@angular/core';
import { CommonModule } from '@angular/common';
`@Component`({
selector: 'app-pagination',
standalone: true,
imports: [CommonModule],
templateUrl: './pagination.component.html',
styleUrl: './pagination.component.css'
})
export class PaginationComponent {
- `@Input`({ required: true }) currentPage!: number;
- `@Input`({ required: true }) totalPages!: number;
- `@Input`() maxVisiblePages = 5;
- `@Input`() pageSize = 50;
- `@Input`() pageSizeOptions = [10, 20, 50, 100, 200];
+ currentPage = input.required<number>();
+ totalPages = input.required<number>();
+ maxVisiblePages = input<number>(5);
+ pageSize = input<number>(50);
+ pageSizeOptions = input<number[]>([10, 20, 50, 100, 200]);Then update the computed body and handlers to read signals (e.g., this.currentPage() instead of this.currentPage).
Option 2: Replace computed with a getter
- pages = computed(() => {
+ get pages(): (number | string)[] {
const pages: (number | string)[] = [];
// ... same logic ...
return pages;
- });
+ }Note: this sacrifices memoization — the getter runs every change-detection cycle.
Also applies to: 21-47
🤖 Prompt for AI Agents
In `@src/Web/src/app/components/shared/pagination/pagination.component.ts` around
lines 12 - 16, The computed() for pages is reading plain `@Input`() properties
(currentPage, totalPages, maxVisiblePages) so it will not update when parents
change; either convert those inputs to writable signals (e.g., change `@Input`()
currentPage to a signal and update call sites to use this.currentPage() /
this.totalPages() / this.maxVisiblePages()) and keep the computed to read the
signals, or replace the computed pages with a plain getter that reads the
current `@Input`() values (remove computed and implement get pages() { ... }) so
it recalculates on each change-detection cycle; update any handlers/uses that
call pages or read currentPage/currentPage() accordingly (refer to currentPage,
totalPages, maxVisiblePages, and the pages computed/getter in
pagination.component.ts).
| getHighlightedChunk(chunk: string, index: number): string { | ||
| const isCurrent = index === this.tts.currentChunkIndex(); | ||
| const charIndex = this.tts.activeCharIndex(); | ||
|
|
||
| if (!isCurrent || charIndex === -1) { | ||
| return chunk; | ||
| } | ||
|
|
||
| // Simple word finding logic starting at charIndex | ||
| const before = chunk.substring(0, charIndex); | ||
| const remainder = chunk.substring(charIndex); | ||
| const wordMatch = remainder.match(/^(\S+)(.*)/s); // Word then rest | ||
|
|
||
| if (wordMatch) { | ||
| const word = wordMatch[1]; | ||
| const after = wordMatch[2]; | ||
| return `${before}<span class="word-highlight">${word}</span>${after}`; | ||
| } | ||
|
|
||
| return chunk; | ||
| } |
There was a problem hiding this comment.
getHighlightedChunk builds raw HTML from unsanitized text — verify rendering context.
This method interpolates chunk content directly into an HTML string. If this is bound via [innerHTML], Angular's built-in sanitizer will strip dangerous tags, but the chunk text itself isn't HTML-escaped — characters like <, >, & in story content could still produce malformed HTML or unexpected rendering.
Consider escaping the text segments before wrapping with the highlight <span>:
Proposed fix
+ private escapeHtml(text: string): string {
+ return text.replace(/&/g, '&').replace(/</g, '<').replace(/>/g, '>');
+ }
+
getHighlightedChunk(chunk: string, index: number): string {
const isCurrent = index === this.tts.currentChunkIndex();
const charIndex = this.tts.activeCharIndex();
if (!isCurrent || charIndex === -1) {
- return chunk;
+ return this.escapeHtml(chunk);
}
const before = chunk.substring(0, charIndex);
const remainder = chunk.substring(charIndex);
const wordMatch = remainder.match(/^(\S+)(.*)/s);
if (wordMatch) {
const word = wordMatch[1];
const after = wordMatch[2];
- return `${before}<span class="word-highlight">${word}</span>${after}`;
+ return `${this.escapeHtml(before)}<span class="word-highlight">${this.escapeHtml(word)}</span>${this.escapeHtml(after)}`;
}
- return chunk;
+ return this.escapeHtml(chunk);
}📝 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.
| getHighlightedChunk(chunk: string, index: number): string { | |
| const isCurrent = index === this.tts.currentChunkIndex(); | |
| const charIndex = this.tts.activeCharIndex(); | |
| if (!isCurrent || charIndex === -1) { | |
| return chunk; | |
| } | |
| // Simple word finding logic starting at charIndex | |
| const before = chunk.substring(0, charIndex); | |
| const remainder = chunk.substring(charIndex); | |
| const wordMatch = remainder.match(/^(\S+)(.*)/s); // Word then rest | |
| if (wordMatch) { | |
| const word = wordMatch[1]; | |
| const after = wordMatch[2]; | |
| return `${before}<span class="word-highlight">${word}</span>${after}`; | |
| } | |
| return chunk; | |
| } | |
| private escapeHtml(text: string): string { | |
| return text.replace(/&/g, '&').replace(/</g, '<').replace(/>/g, '>'); | |
| } | |
| getHighlightedChunk(chunk: string, index: number): string { | |
| const isCurrent = index === this.tts.currentChunkIndex(); | |
| const charIndex = this.tts.activeCharIndex(); | |
| if (!isCurrent || charIndex === -1) { | |
| return this.escapeHtml(chunk); | |
| } | |
| // Simple word finding logic starting at charIndex | |
| const before = chunk.substring(0, charIndex); | |
| const remainder = chunk.substring(charIndex); | |
| const wordMatch = remainder.match(/^(\S+)(.*)/s); // Word then rest | |
| if (wordMatch) { | |
| const word = wordMatch[1]; | |
| const after = wordMatch[2]; | |
| return `${this.escapeHtml(before)}<span class="word-highlight">${this.escapeHtml(word)}</span>${this.escapeHtml(after)}`; | |
| } | |
| return this.escapeHtml(chunk); | |
| } |
🤖 Prompt for AI Agents
In `@src/Web/src/app/components/story-reader/story-reader.component.ts` around
lines 97 - 117, getHighlightedChunk builds raw HTML by concatenating the
incoming chunk and highlighted word without escaping, which can create malformed
HTML or XSS when chunk contains `<`, `>`, `&`; update getHighlightedChunk to
HTML-escape the text segments before composing the string (escape the computed
before, word and after parts) or alternatively build the span using DOM/text
APIs (e.g., create a span with textContent) and return safe HTML; locate
getHighlightedChunk (uses this.tts.currentChunkIndex() and
this.tts.activeCharIndex() and the "word-highlight" span) and ensure the
returned value is safe for binding via [innerHTML] (or switch the template to
render nodes instead of raw HTML) so no unescaped chunk text is inserted.
| // Backend API base URL | ||
| private static apiBase = 'http://localhost:5285/api/tts'; |
There was a problem hiding this comment.
Hardcoded localhost URL will break outside local development.
apiBase is hardcoded to http://localhost:5285/api/tts. This will fail in staging, production, or any deployment where the API host differs. Use Angular's environment configuration or a relative URL (e.g., /api/tts) so the request goes to the same origin.
🐛 Proposed fix (relative URL)
- private static apiBase = 'http://localhost:5285/api/tts';
+ private static apiBase = '/api/tts';📝 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.
| // Backend API base URL | |
| private static apiBase = 'http://localhost:5285/api/tts'; | |
| // Backend API base URL | |
| private static apiBase = '/api/tts'; |
🤖 Prompt for AI Agents
In `@src/Web/src/app/services/tts/cloud-tts-engine.ts` around lines 34 - 35, The
static apiBase field in CloudTtsEngine is hardcoded to
'http://localhost:5285/api/tts' which breaks non-local deployments; change it to
use the Angular environment or a relative path: replace the value of private
static apiBase with something like environment.apiBase + '/api/tts' (importing
the environment) or simply '/api/tts' as a fallback so requests go to the same
origin, and ensure any code referencing CloudTtsEngine.apiBase continues to work
with the new value.
| static stop() { | ||
| this.currentRequestId++; // Invalidate pending synthesis requests | ||
| if (this.audio) { | ||
| this.audio.pause(); | ||
| this.audio.onended = null; // Prevent memory leaks | ||
| this.audio.onerror = null; | ||
| this.audio = null; | ||
| } | ||
| this.isCurrentlySpeaking = false; | ||
| this.isCurrentlyPaused = false; | ||
| } |
There was a problem hiding this comment.
Object URL memory leak when stop() is called during playback.
stop() nullifies onended/onerror (the handlers that call URL.revokeObjectURL) and then sets this.audio = null, but never revokes the Object URL itself. Every stop() during active playback leaks a blob URL.
🐛 Proposed fix
static stop() {
this.currentRequestId++;
if (this.audio) {
+ const src = this.audio.src;
this.audio.pause();
this.audio.onended = null;
this.audio.onerror = null;
+ if (src.startsWith('blob:')) {
+ URL.revokeObjectURL(src);
+ }
this.audio = null;
}
this.isCurrentlySpeaking = false;
this.isCurrentlyPaused = false;
}📝 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.
| static stop() { | |
| this.currentRequestId++; // Invalidate pending synthesis requests | |
| if (this.audio) { | |
| this.audio.pause(); | |
| this.audio.onended = null; // Prevent memory leaks | |
| this.audio.onerror = null; | |
| this.audio = null; | |
| } | |
| this.isCurrentlySpeaking = false; | |
| this.isCurrentlyPaused = false; | |
| } | |
| static stop() { | |
| this.currentRequestId++; // Invalidate pending synthesis requests | |
| if (this.audio) { | |
| const src = this.audio.src; | |
| this.audio.pause(); | |
| this.audio.onended = null; // Prevent memory leaks | |
| this.audio.onerror = null; | |
| if (src.startsWith('blob:')) { | |
| URL.revokeObjectURL(src); | |
| } | |
| this.audio = null; | |
| } | |
| this.isCurrentlySpeaking = false; | |
| this.isCurrentlyPaused = false; | |
| } |
🤖 Prompt for AI Agents
In `@src/Web/src/app/services/tts/cloud-tts-engine.ts` around lines 182 - 192, The
stop() method in cloud-tts-engine.ts currently nulls audio handlers and the
audio object but never revokes the blob URL, leaking object URLs; update stop()
(static) to, before dropping this.audio, check if this.audio.src is a blob URL
(starts with "blob:") and call URL.revokeObjectURL(this.audio.src), then remove
handlers (this.audio.onended/this.audio.onerror = null) and finally set
this.audio = null; keep incrementing currentRequestId and resetting
isCurrentlySpeaking/isCurrentlyPaused as-is. Ensure you reference the static
stop() method and the this.audio.src property when making the change.
| const nativeVoices: TtsVoice[] = this.synth.getVoices().map(v => ({ | ||
| name: v.name, | ||
| voiceURI: v.voiceURI, | ||
| lang: v.lang, | ||
| provider: 'native' | ||
| })); |
There was a problem hiding this comment.
getVoices() may return an empty array on the first call.
In most browsers, speechSynthesis.getVoices() returns [] until the voiceschanged event fires. If initVoices is called before that event, native voices will be silently missing. You need to wait for the voiceschanged event or retry.
🛠️ Proposed fix: wait for voices to load
+ private getNativeVoices(): Promise<SpeechSynthesisVoice[]> {
+ return new Promise((resolve) => {
+ const voices = this.synth.getVoices();
+ if (voices.length > 0) {
+ resolve(voices);
+ return;
+ }
+ const handler = () => {
+ this.synth.removeEventListener('voiceschanged', handler);
+ resolve(this.synth.getVoices());
+ };
+ this.synth.addEventListener('voiceschanged', handler);
+ });
+ }Then in initVoices:
- const nativeVoices: TtsVoice[] = this.synth.getVoices().map(v => ({
+ const nativeVoices: TtsVoice[] = (await this.getNativeVoices()).map(v => ({🤖 Prompt for AI Agents
In `@src/Web/src/app/services/tts/tts-voice-manager.ts` around lines 27 - 32, The
native voice list is read via this.synth.getVoices() (mapped into TtsVoice) but
getVoices() can return [] initially; modify initVoices to wait for voices to
load by checking this.synth.getVoices() and if empty attach a one-time
'voiceschanged' listener (or retry with a small timeout) before mapping into
nativeVoices, then proceed to merge with other providers. Ensure you reference
this.synth.getVoices(), the initVoices routine, and the TtsVoice mapping so the
code only builds nativeVoices after voices are available.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests