Website JavaScript Modules from PR #169 (Part 2/2)#190
Conversation
Fortress-compliant implementation (5/5 files): - comprehensive-demo.js: Core demo functionality with DeBERTa v3 Large - config.js: Configuration management - demo-initialization.js: Demo setup and initialization - layout-manager.js: UI layout and responsive design - voice-recorder.js: Voice recording and transcription Part 2/2 of sophisticated website files from feat/clean-demo-website. Enables full interactive emotion detection demo with 28 emotions. Original work attribution: PR #169 feat/clean-demo-website 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
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. WalkthroughThis change set introduces a centralized, client-side configuration module and a modular structure to the web demo client. It implements configuration-driven API endpoints, robust state and layout management, a comprehensive AI workflow client, a voice recording and transcription module, and enhanced UI orchestration for interactions. Multiple new JavaScript modules and HTML integrations are included. Changes
Sequence Diagram(s)sequenceDiagram
%% Modular Demo Client Core Workflow
participant User as User
participant UI as UI/DOM
participant Config as config.js (SAMO_CONFIG)
participant Client as SAMOAPIClient
participant Recorder as VoiceRecorder
participant Layout as LayoutManager
participant API as API Server
%% Initialization/Config
Note left of UI: (DOMContentLoaded)
UI->>Config: Load config.js, set SAMO_CONFIG
Config-->>UI: Expose config, possibly merge with server config
%% Processing Workflow
User->>UI: Click "Process"
UI->>Layout: showProcessingState()\n(track request, lock UI)
UI->>Client: processCompleteWorkflow(input)
Client->>API: /transcribe (if needed)
API-->>Client: return transcription result
Client->>API: /summarize
API-->>Client: summary result
Client->>API: /emotion
API-->>Client: emotion results
Client-->>UI: Results payload
UI->>Layout: showResultsState()
UI->>UI: Render summary, emotions, etc.
%% Voice Recorder
User->>Recorder: Click "Record"
Recorder->>UI: Start Timer, update UI
Recorder->>Recorder: MediaRecorder.start
User->>Recorder: Click "Stop"
Recorder->>Recorder: MediaRecorder.stop, create Blob
Recorder->>Client: transcribeAudio(blob)
Client->>API: /transcribe (audio)
API-->>Client: transcription
Client-->>Recorder: Return text result
Recorder->>UI: Display transcription
%% Error Handling and Fallback
alt API errors or rate limiting
Client-->>UI: Show mock/fallback data, showError
Layout->>Layout: forceResetProcessing/emergencyReset
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
Reviewer's GuideThis PR modularizes the interactive emotion detection demo into ES6-compliant JavaScript modules, introducing a robust API client, voice recording support, enhanced UI state management, centralized configuration, and a lightweight initialization script to orchestrate demo flows. Sequence diagram for user voice recording and emotion analysis workflowsequenceDiagram
actor User
participant "VoiceRecorder"
participant "SAMOAPIClient"
participant "LayoutManager"
participant "UI Elements"
User->>"VoiceRecorder": Clicks recordBtn
"VoiceRecorder"->>"VoiceRecorder": startRecording()
User->>"VoiceRecorder": Clicks stopBtn
"VoiceRecorder"->>"VoiceRecorder": stopRecording()
"VoiceRecorder"->>"VoiceRecorder": onRecordingStop()
"VoiceRecorder"->>"SAMOAPIClient": transcribeAudio(audioFile)
"SAMOAPIClient"->>"SAMOAPIClient": makeRequest(VOICE_JOURNAL)
"SAMOAPIClient"->>"SAMOAPIClient": process response
"SAMOAPIClient"-->>"VoiceRecorder": transcription result
"VoiceRecorder"->>"UI Elements": displayTranscriptionResults(result)
"VoiceRecorder"->>"LayoutManager": showProcessingState()
"VoiceRecorder"->>"SAMOAPIClient": (optional) processCompleteWorkflow(audioFile, text)
"SAMOAPIClient"->>"SAMOAPIClient": summarizeText(), detectEmotions()
"SAMOAPIClient"-->>"VoiceRecorder": analysis results
"VoiceRecorder"->>"UI Elements": update UI with results
"VoiceRecorder"->>"LayoutManager": showResultsState()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Here's the code health analysis summary for commits Analysis Summary
|
Summary of ChangesHello @uelkerd, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers the interactive components for the website's AI platform demo, completing the work initiated in PR #169. It establishes a sophisticated client-side application capable of voice transcription, text summarization, and detailed emotion detection using the DeBERTa v3 Large model. The implementation emphasizes modularity, robust API interaction, dynamic UI management, and user-friendly features like AI-generated sample text and real-time processing feedback. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements sophisticated interactive JavaScript modules for the SAMO AI platform demo, supporting voice recording, transcription, and emotion detection with 28 emotions. It serves as Part 2/2 of the comprehensive website implementation, introducing advanced client-side functionality to complement the static website files.
Key Changes
- Interactive voice recording and transcription capabilities with comprehensive error handling
- Responsive layout management system with state transitions and processing guards
- Comprehensive AI processing pipeline integrating DeBERTa v3 Large for emotion analysis
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| website/js/voice-recorder.js | Voice recording module with microphone access, transcription, and API integration |
| website/js/layout-manager.js | UI state management for processing transitions and responsive design |
| website/js/demo-initialization.js | DOM initialization with event listeners and Bootstrap tooltip setup |
| website/js/config.js | Centralized configuration management for API endpoints and environment settings |
| website/js/comprehensive-demo.js | Main demo functionality with API client, emotion detection, and visualization |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| async processRecordedAudio(audioBlob) { | ||
| try { | ||
| // Show processing state | ||
| this.showProcessingState(); | ||
|
|
||
| // Create a File object from the blob | ||
| const audioFile = new File([audioBlob], 'recording.webm', { |
There was a problem hiding this comment.
The filename extension 'recording.webm' is hardcoded but may not match the actual MIME type returned by getSupportedMimeType(). This could cause issues with audio processing on the server side. Consider using a dynamic extension based on the blob's MIME type.
| async processRecordedAudio(audioBlob) { | |
| try { | |
| // Show processing state | |
| this.showProcessingState(); | |
| // Create a File object from the blob | |
| const audioFile = new File([audioBlob], 'recording.webm', { | |
| // Helper to map MIME type to file extension | |
| getExtensionFromMimeType(mimeType) { | |
| const mimeToExt = { | |
| 'audio/webm': 'webm', | |
| 'audio/ogg': 'ogg', | |
| 'audio/wav': 'wav', | |
| 'audio/mp3': 'mp3', | |
| 'audio/mpeg': 'mp3', | |
| 'audio/x-wav': 'wav', | |
| 'audio/mp4': 'mp4', | |
| }; | |
| return mimeToExt[mimeType] || 'bin'; | |
| } | |
| async processRecordedAudio(audioBlob) { | |
| try { | |
| // Show processing state | |
| this.showProcessingState(); | |
| // Create a File object from the blob with dynamic extension | |
| const extension = this.getExtensionFromMimeType(audioBlob.type); | |
| const filename = `recording.${extension}`; | |
| const audioFile = new File([audioBlob], filename, { |
| if (this.isProcessing) { | ||
| // Check if processing has been stuck for too long | ||
| const timeElapsed = Date.now() - this.processingStartTime; | ||
| if (timeElapsed > this.maxProcessingTime) { | ||
| console.warn(`⚠️ Processing stuck for ${timeElapsed/1000}s, forcing reset...`); | ||
| this.forceResetProcessing(); | ||
| } else { | ||
| console.warn('⚠️ Processing already in progress, ignoring request'); | ||
| console.warn('⚠️ Current state:', this.currentState); | ||
| console.warn('⚠️ Active requests:', this.activeRequests.size); | ||
| console.warn(`⚠️ Time elapsed: ${timeElapsed/1000}s`); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential null reference error when processingStartTime is null. If isProcessing is true but processingStartTime hasn't been set, Date.now() - null will result in NaN, causing the comparison timeElapsed > this.maxProcessingTime to always be false.
| this.timeout = window.SAMO_CONFIG?.API?.TIMEOUT || 15000; // Reduced from 20s to 15s | ||
| this.coldStartTimeout = window.SAMO_CONFIG?.API?.COLD_START_TIMEOUT || 45000; // Reduced from 60s to 45s |
There was a problem hiding this comment.
The comments mention reductions from previous values (20s to 15s, 60s to 45s), but these previous values are not the current fallback values in the code. The comments should either be updated to reflect the actual current fallback values or removed to avoid confusion.
| this.timeout = window.SAMO_CONFIG?.API?.TIMEOUT || 15000; // Reduced from 20s to 15s | |
| this.coldStartTimeout = window.SAMO_CONFIG?.API?.COLD_START_TIMEOUT || 45000; // Reduced from 60s to 45s | |
| this.timeout = window.SAMO_CONFIG?.API?.TIMEOUT || 15000; // Fallback: 15s | |
| this.coldStartTimeout = window.SAMO_CONFIG?.API?.COLD_START_TIMEOUT || 45000; // Fallback: 45s |
| // Create API client instance for proper timeout and error handling | ||
| const apiClient = new SAMOAPIClient(); |
There was a problem hiding this comment.
A new SAMOAPIClient instance is created on line 768 when a global apiClient instance already exists. This creates unnecessary overhead and doesn't leverage the existing cold start tracking. Consider using the global window.apiClient instance instead.
| // Create API client instance for proper timeout and error handling | |
| const apiClient = new SAMOAPIClient(); | |
| // Use global API client instance for proper timeout and error handling | |
| const apiClient = window.apiClient; |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider breaking comprehensive-demo.js into focused modules (e.g., API client, mock data, workflow) to improve maintainability and testability.
- Many error handling and retry patterns in SAMOAPIClient are duplicated; extracting common utilities or middleware could reduce redundancy.
- The code currently attaches numerous globals to window, which may lead to namespace collisions—consider leveraging ES6 imports/exports or a dedicated namespace object.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider breaking comprehensive-demo.js into focused modules (e.g., API client, mock data, workflow) to improve maintainability and testability.
- Many error handling and retry patterns in SAMOAPIClient are duplicated; extracting common utilities or middleware could reduce redundancy.
- The code currently attaches numerous globals to window, which may lead to namespace collisions—consider leveraging ES6 imports/exports or a dedicated namespace object.
## Individual Comments
### Comment 1
<location> `website/js/voice-recorder.js:175-176` </location>
<code_context>
+ }
+ }
+
+ // Use API client to transcribe
+ if (apiClient && typeof apiClient.transcribeAudio === 'function') {
+ console.log('🔄 Sending audio for transcription...');
+ const response = await apiClient.transcribeAudio(audioFile);
</code_context>
<issue_to_address>
**issue (bug_risk):** API client response handling may be inconsistent with SAMOAPIClient's return type.
The code assumes apiClient.transcribeAudio returns a Response object with an 'ok' property, but it actually returns parsed JSON or throws on error. Update the response handling to match the actual return type to avoid runtime issues.
</issue_to_address>
### Comment 2
<location> `website/js/voice-recorder.js:247-249` </location>
<code_context>
+ }
+
+ // If we have complete analysis results, display them
+ if (result.emotion_analysis || result.summary) {
+ // Trigger the processing to show results
+ if (typeof processTextWithStateManagement === 'function') {
+ processTextWithStateManagement();
+ } else if (typeof processText === 'function') {
</code_context>
<issue_to_address>
**suggestion (performance):** Triggering processText after transcription may result in duplicate or unnecessary processing.
If emotion_analysis and summary are already provided, avoid re-calling processText to prevent redundant API usage and UI updates. Refactor to use the existing results.
Suggested implementation:
```javascript
// If we have complete analysis results, display them
if (result.emotion_analysis || result.summary) {
// Use existing results to update the UI, avoid redundant processing
if (typeof displayAnalysisResults === 'function') {
displayAnalysisResults(result.emotion_analysis, result.summary);
} else {
// Fallback: directly update UI elements if displayAnalysisResults is not defined
if (result.emotion_analysis && document.getElementById('emotionAnalysis')) {
document.getElementById('emotionAnalysis').textContent = JSON.stringify(result.emotion_analysis);
}
if (result.summary && document.getElementById('summary')) {
document.getElementById('summary').textContent = result.summary;
}
}
}
```
- Ensure that a function named `displayAnalysisResults(emotionAnalysis, summary)` exists elsewhere in your codebase to handle UI updates, or adjust the fallback logic to match your UI structure.
- If you have custom UI update logic for emotion analysis and summary, replace the fallback code with your own implementation.
- Remove or refactor any code that depends on `processTextWithStateManagement` or `processText` being called after transcription if it is no longer needed.
</issue_to_address>
### Comment 3
<location> `website/js/comprehensive-demo.js:73-82` </location>
<code_context>
+ return params.toString();
+ }
+
+ async makeRequestWithRetry(endpoint, data, method = 'POST', isFormData = false, timeoutMs = null, attemptsLeft = null) {
+ // Use class defaults if not specified
+ if (attemptsLeft === null) attemptsLeft = this.retryAttempts;
+
+ const config = {
+ method,
+ headers: {}
+ };
+ const controller = new AbortController();
+
+ // Use cold start timeout for first request, regular timeout otherwise
+ const timeout = timeoutMs || (this.isColdStart ? this.coldStartTimeout : this.timeout);
+ const timer = setTimeout(() => {
+ controller.abort(new Error(`Request timeout after ${timeout/1000}s`));
+ }, timeout);
</code_context>
<issue_to_address>
**suggestion:** AbortController usage may not work as intended with fetch and custom error.
Note that fetch will always throw an AbortError when aborted, regardless of the error passed to controller.abort(). To differentiate abort reasons, track them separately.
Suggested implementation:
```javascript
const controller = new AbortController();
let abortReason = null; // Track abort reason
// Use cold start timeout for first request, regular timeout otherwise
const timeout = timeoutMs || (this.isColdStart ? this.coldStartTimeout : this.timeout);
const timer = setTimeout(() => {
abortReason = 'timeout';
controller.abort();
}, timeout);
config.signal = controller.signal;
```
To fully implement this, you must also:
1. Update the code that catches errors from `fetch` to check the `abortReason` variable. If `abortReason === 'timeout'`, you can handle the timeout case specifically (e.g., by logging or throwing a custom error).
2. Remove any reliance on the error passed to `controller.abort()` elsewhere in the code.
</issue_to_address>
### Comment 4
<location> `website/js/comprehensive-demo.js:101-102` </location>
<code_context>
+ config.headers['X-API-Key'] = apiKey;
+ }
+
+ if (data && method === 'POST') {
+ if (isFormData) {
+ // For FormData, don't set Content-Type header - let browser set it with boundary
+ config.body = data;
</code_context>
<issue_to_address>
**suggestion:** No check for GET requests with data payload.
For GET requests with data, build a query string from the data and append it to the URL.
</issue_to_address>
### Comment 5
<location> `website/js/layout-manager.js:284-285` </location>
<code_context>
+ });
+ },
+
+ // Update progress steps
+ updateProgressStep(stepNumber, state) {
+ // Update both horizontal and vertical progress indicators
+ const stepElement = document.getElementById(`step${stepNumber}`);
</code_context>
<issue_to_address>
**suggestion:** updateProgressStep assumes existence of step elements and icons.
Add a warning or error log if step elements or icons are missing to improve debuggability.
</issue_to_address>
### Comment 6
<location> `website/js/demo-initialization.js:117-124` </location>
<code_context>
+ window.LayoutManager.resetToInitialState();
+ }
+
+ // Initialize Bootstrap tooltips
+ const tooltipTriggerList = [].slice.call(document.querySelectorAll('[data-bs-toggle="tooltip"]'));
+ const tooltipList = tooltipTriggerList.map(function (tooltipTriggerEl) {
+ return new bootstrap.Tooltip(tooltipTriggerEl);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Bootstrap tooltips initialization may fail if Bootstrap is not loaded.
Check for window.bootstrap before initializing tooltips to prevent ReferenceError if Bootstrap is not loaded.
```suggestion
// Initialize Bootstrap tooltips if Bootstrap is loaded
let tooltipList = [];
if (window.bootstrap) {
const tooltipTriggerList = [].slice.call(document.querySelectorAll('[data-bs-toggle="tooltip"]'));
tooltipList = tooltipTriggerList.map(function (tooltipTriggerEl) {
return new bootstrap.Tooltip(tooltipTriggerEl);
});
console.log('✅ Bootstrap tooltips initialized:', tooltipList.length);
} else {
console.warn('⚠️ Bootstrap is not loaded. Tooltips not initialized.');
}
console.log('✅ Enhanced layout manager initialized');
```
</issue_to_address>
### Comment 7
<location> `website/js/comprehensive-demo.js:134-147` </location>
<code_context>
if (response.status === 429 || response.status >= 500) {
if (attemptsLeft > 1) {
const backoffDelay = Math.pow(2, this.retryAttempts - attemptsLeft) * 1000; // Exponential backoff
console.warn(`Request failed (${response.status}), retrying in ${backoffDelay}ms. Attempts left: ${attemptsLeft - 1}`);
// Provide user feedback about retry
if (typeof addToProgressConsole === 'function') {
addToProgressConsole(`Request failed (${response.status}), retrying in ${backoffDelay/1000}s...`, 'warning');
}
await new Promise(resolve => setTimeout(resolve, backoffDelay));
return this.makeRequestWithRetry(endpoint, data, method, isFormData, timeoutMs, attemptsLeft - 1);
}
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/merge-nested-ifs))
```suggestion
if ((response.status === 429 || response.status >= 500) && attemptsLeft > 1) {
const backoffDelay = Math.pow(2, this.retryAttempts - attemptsLeft) * 1000; // Exponential backoff
console.warn(`Request failed (${response.status}), retrying in ${backoffDelay}ms. Attempts left: ${attemptsLeft - 1}`);
// Provide user feedback about retry
if (typeof addToProgressConsole === 'function') {
addToProgressConsole(`Request failed (${response.status}), retrying in ${backoffDelay/1000}s...`, 'warning');
}
await new Promise(resolve => setTimeout(resolve, backoffDelay));
return this.makeRequestWithRetry(endpoint, data, method, isFormData, timeoutMs, attemptsLeft - 1);
}
```
<br/><details><summary>Explanation</summary>Reading deeply nested conditional code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 8
<location> `website/js/comprehensive-demo.js:206-209` </location>
<code_context>
const response = await this.makeRequest(this.endpoints.SUMMARIZE, { text }, 'POST');
// The makeRequest method already handles JSON parsing, so response is the data
return response;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/inline-immediately-returned-variable))
```suggestion
return await this.makeRequest(this.endpoints.SUMMARIZE, { text }, 'POST');
```
<br/><details><summary>Explanation</summary>Something that we often see in people's code is assigning to a result variable
and then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
</details>
</issue_to_address>
### Comment 9
<location> `website/js/voice-recorder.js:159` </location>
<code_context>
let apiClient = window.apiClient;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/use-object-destructuring))
```suggestion
let {apiClient} = window;
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // Use API client to transcribe | ||
| if (apiClient && typeof apiClient.transcribeAudio === 'function') { |
There was a problem hiding this comment.
issue (bug_risk): API client response handling may be inconsistent with SAMOAPIClient's return type.
The code assumes apiClient.transcribeAudio returns a Response object with an 'ok' property, but it actually returns parsed JSON or throws on error. Update the response handling to match the actual return type to avoid runtime issues.
| if (result.emotion_analysis || result.summary) { | ||
| // Trigger the processing to show results | ||
| if (typeof processTextWithStateManagement === 'function') { |
There was a problem hiding this comment.
suggestion (performance): Triggering processText after transcription may result in duplicate or unnecessary processing.
If emotion_analysis and summary are already provided, avoid re-calling processText to prevent redundant API usage and UI updates. Refactor to use the existing results.
Suggested implementation:
// If we have complete analysis results, display them
if (result.emotion_analysis || result.summary) {
// Use existing results to update the UI, avoid redundant processing
if (typeof displayAnalysisResults === 'function') {
displayAnalysisResults(result.emotion_analysis, result.summary);
} else {
// Fallback: directly update UI elements if displayAnalysisResults is not defined
if (result.emotion_analysis && document.getElementById('emotionAnalysis')) {
document.getElementById('emotionAnalysis').textContent = JSON.stringify(result.emotion_analysis);
}
if (result.summary && document.getElementById('summary')) {
document.getElementById('summary').textContent = result.summary;
}
}
}- Ensure that a function named
displayAnalysisResults(emotionAnalysis, summary)exists elsewhere in your codebase to handle UI updates, or adjust the fallback logic to match your UI structure. - If you have custom UI update logic for emotion analysis and summary, replace the fallback code with your own implementation.
- Remove or refactor any code that depends on
processTextWithStateManagementorprocessTextbeing called after transcription if it is no longer needed.
| async makeRequestWithRetry(endpoint, data, method = 'POST', isFormData = false, timeoutMs = null, attemptsLeft = null) { | ||
| // Use class defaults if not specified | ||
| if (attemptsLeft === null) attemptsLeft = this.retryAttempts; | ||
|
|
||
| const config = { | ||
| method, | ||
| headers: {} | ||
| }; | ||
| const controller = new AbortController(); | ||
|
|
There was a problem hiding this comment.
suggestion: AbortController usage may not work as intended with fetch and custom error.
Note that fetch will always throw an AbortError when aborted, regardless of the error passed to controller.abort(). To differentiate abort reasons, track them separately.
Suggested implementation:
const controller = new AbortController();
let abortReason = null; // Track abort reason
// Use cold start timeout for first request, regular timeout otherwise
const timeout = timeoutMs || (this.isColdStart ? this.coldStartTimeout : this.timeout);
const timer = setTimeout(() => {
abortReason = 'timeout';
controller.abort();
}, timeout);
config.signal = controller.signal;To fully implement this, you must also:
- Update the code that catches errors from
fetchto check theabortReasonvariable. IfabortReason === 'timeout', you can handle the timeout case specifically (e.g., by logging or throwing a custom error). - Remove any reliance on the error passed to
controller.abort()elsewhere in the code.
| if (data && method === 'POST') { | ||
| if (isFormData) { |
There was a problem hiding this comment.
suggestion: No check for GET requests with data payload.
For GET requests with data, build a query string from the data and append it to the URL.
| // Update progress steps | ||
| updateProgressStep(stepNumber, state) { |
There was a problem hiding this comment.
suggestion: updateProgressStep assumes existence of step elements and icons.
Add a warning or error log if step elements or icons are missing to improve debuggability.
| // Initialize Bootstrap tooltips | ||
| const tooltipTriggerList = [].slice.call(document.querySelectorAll('[data-bs-toggle="tooltip"]')); | ||
| const tooltipList = tooltipTriggerList.map(function (tooltipTriggerEl) { | ||
| return new bootstrap.Tooltip(tooltipTriggerEl); | ||
| }); | ||
|
|
||
| console.log('✅ Enhanced layout manager initialized'); | ||
| console.log('✅ Bootstrap tooltips initialized:', tooltipList.length); |
There was a problem hiding this comment.
suggestion (bug_risk): Bootstrap tooltips initialization may fail if Bootstrap is not loaded.
Check for window.bootstrap before initializing tooltips to prevent ReferenceError if Bootstrap is not loaded.
| // Initialize Bootstrap tooltips | |
| const tooltipTriggerList = [].slice.call(document.querySelectorAll('[data-bs-toggle="tooltip"]')); | |
| const tooltipList = tooltipTriggerList.map(function (tooltipTriggerEl) { | |
| return new bootstrap.Tooltip(tooltipTriggerEl); | |
| }); | |
| console.log('✅ Enhanced layout manager initialized'); | |
| console.log('✅ Bootstrap tooltips initialized:', tooltipList.length); | |
| // Initialize Bootstrap tooltips if Bootstrap is loaded | |
| let tooltipList = []; | |
| if (window.bootstrap) { | |
| const tooltipTriggerList = [].slice.call(document.querySelectorAll('[data-bs-toggle="tooltip"]')); | |
| tooltipList = tooltipTriggerList.map(function (tooltipTriggerEl) { | |
| return new bootstrap.Tooltip(tooltipTriggerEl); | |
| }); | |
| console.log('✅ Bootstrap tooltips initialized:', tooltipList.length); | |
| } else { | |
| console.warn('⚠️ Bootstrap is not loaded. Tooltips not initialized.'); | |
| } | |
| console.log('✅ Enhanced layout manager initialized'); |
| if (response.status === 429 || response.status >= 500) { | ||
| if (attemptsLeft > 1) { | ||
| const backoffDelay = Math.pow(2, this.retryAttempts - attemptsLeft) * 1000; // Exponential backoff | ||
| console.warn(`Request failed (${response.status}), retrying in ${backoffDelay}ms. Attempts left: ${attemptsLeft - 1}`); | ||
|
|
||
| // Provide user feedback about retry | ||
| if (typeof addToProgressConsole === 'function') { | ||
| addToProgressConsole(`Request failed (${response.status}), retrying in ${backoffDelay/1000}s...`, 'warning'); | ||
| } | ||
|
|
||
| await new Promise(resolve => setTimeout(resolve, backoffDelay)); | ||
| return this.makeRequestWithRetry(endpoint, data, method, isFormData, timeoutMs, attemptsLeft - 1); | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
| if (response.status === 429 || response.status >= 500) { | |
| if (attemptsLeft > 1) { | |
| const backoffDelay = Math.pow(2, this.retryAttempts - attemptsLeft) * 1000; // Exponential backoff | |
| console.warn(`Request failed (${response.status}), retrying in ${backoffDelay}ms. Attempts left: ${attemptsLeft - 1}`); | |
| // Provide user feedback about retry | |
| if (typeof addToProgressConsole === 'function') { | |
| addToProgressConsole(`Request failed (${response.status}), retrying in ${backoffDelay/1000}s...`, 'warning'); | |
| } | |
| await new Promise(resolve => setTimeout(resolve, backoffDelay)); | |
| return this.makeRequestWithRetry(endpoint, data, method, isFormData, timeoutMs, attemptsLeft - 1); | |
| } | |
| } | |
| if ((response.status === 429 || response.status >= 500) && attemptsLeft > 1) { | |
| const backoffDelay = Math.pow(2, this.retryAttempts - attemptsLeft) * 1000; // Exponential backoff | |
| console.warn(`Request failed (${response.status}), retrying in ${backoffDelay}ms. Attempts left: ${attemptsLeft - 1}`); | |
| // Provide user feedback about retry | |
| if (typeof addToProgressConsole === 'function') { | |
| addToProgressConsole(`Request failed (${response.status}), retrying in ${backoffDelay/1000}s...`, 'warning'); | |
| } | |
| await new Promise(resolve => setTimeout(resolve, backoffDelay)); | |
| return this.makeRequestWithRetry(endpoint, data, method, isFormData, timeoutMs, attemptsLeft - 1); | |
| } | |
Explanation
Reading deeply nested conditional code is confusing, since you have to keep track of whichconditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two
if conditions can be combined usingand is an easy win.
| const response = await this.makeRequest(this.endpoints.SUMMARIZE, { text }, 'POST'); | ||
|
|
||
| // The makeRequest method already handles JSON parsing, so response is the data | ||
| return response; |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| const response = await this.makeRequest(this.endpoints.SUMMARIZE, { text }, 'POST'); | |
| // The makeRequest method already handles JSON parsing, so response is the data | |
| return response; | |
| return await this.makeRequest(this.endpoints.SUMMARIZE, { text }, 'POST'); | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
| }); | ||
|
|
||
| // Get or create API client | ||
| let apiClient = window.apiClient; |
There was a problem hiding this comment.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
| let apiClient = window.apiClient; | |
| let {apiClient} = window; |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several new JavaScript modules to build an interactive demo website. The code is well-structured with a clear separation of concerns into modules for API communication, configuration, layout management, and voice recording. The use of classes and modern JavaScript features is commendable.
However, I've identified a few areas for improvement. There's a critical bug in voice-recorder.js where the API response is handled incorrectly, which will prevent voice transcription from working. I've also found several instances of redundant code, such as re-creating the API client instead of using the global instance, and duplicated UI notification logic. Some parts of the code could be made more robust and maintainable by better leveraging the existing SAMOAPIClient for all API calls, including the one to OpenAI. Finally, there are some minor issues like dead code and a misleading log message.
My feedback focuses on fixing the critical bug, reducing redundancy, and improving the overall consistency and maintainability of the new modules.
| const response = await apiClient.transcribeAudio(audioFile); | ||
|
|
||
| if (response.ok) { | ||
| const result = await response.json(); | ||
| console.log('✅ Transcription successful:', result); | ||
|
|
||
| // Display results in the UI | ||
| this.displayTranscriptionResults(result); | ||
| } else { | ||
| const errorText = await response.text().catch(() => 'Unknown error'); | ||
| throw new Error(`API request failed (${response.status}): ${errorText}`); | ||
| } |
There was a problem hiding this comment.
There is a critical issue in how the response from apiClient.transcribeAudio(audioFile) is handled. The apiClient.makeRequest method (used by transcribeAudio) already processes the fetch response, checks for errors, and returns the parsed JSON data. However, this code treats the returned data as a raw Response object, incorrectly checking response.ok and calling response.json(). This will cause the transcription to fail. You should directly use the response variable as it already contains the final JSON result.
console.log('🔄 Sending audio for transcription...');
const result = await apiClient.transcribeAudio(audioFile);
console.log('✅ Transcription successful:', result);
// Display results in the UI
this.displayTranscriptionResults(result);| console.log('🔥 Calling emotion API...'); | ||
| addToProgressConsole('🌐 Sending request to emotion analysis API...', 'processing'); | ||
| // Create API client instance for proper timeout and error handling | ||
| const apiClient = new SAMOAPIClient(); |
There was a problem hiding this comment.
A new SAMOAPIClient is instantiated here. This is inefficient and bypasses the stateful features of the global window.apiClient instance (like the isColdStart optimization). You should use the globally-initialized window.apiClient instead.
| const apiClient = new SAMOAPIClient(); | |
| const apiClient = window.apiClient; |
| addToProgressConsole('🌐 Sending request to emotion analysis API...', 'processing'); | ||
| // Create API client instance for proper timeout and error handling | ||
| const apiClient = new SAMOAPIClient(); | ||
| const data = await apiClient.makeRequest(apiClient.endpoints.EMOTION, { text: testText }, 'POST'); |
There was a problem hiding this comment.
Instead of calling makeRequest directly, you should use the higher-level detectEmotions method. This encapsulates the endpoint logic and includes valuable features like the mock data fallback, making the code more robust and maintainable.
| const data = await apiClient.makeRequest(apiClient.endpoints.EMOTION, { text: testText }, 'POST'); | |
| const data = await apiClient.detectEmotions(testText); |
|
|
||
| try { | ||
| // Create API client instance for proper timeout and error handling | ||
| const apiClient = new SAMOAPIClient(); |
There was a problem hiding this comment.
A new SAMOAPIClient is instantiated here, similar to the issue in testWithRealAPI. This is inefficient and bypasses the stateful features of the global window.apiClient instance. You should use the globally-initialized window.apiClient instead.
| const apiClient = new SAMOAPIClient(); | |
| const apiClient = window.apiClient; |
| console.log('📝 Summarization endpoint:', apiClient.endpoints.SUMMARIZE); | ||
| console.log('📝 Full API URL:', `${apiClient.baseURL}${apiClient.endpoints.SUMMARIZE}`); | ||
|
|
||
| const data = await apiClient.makeRequest(apiClient.endpoints.SUMMARIZE, { text: text }, 'POST'); |
There was a problem hiding this comment.
Instead of calling makeRequest directly, you should use the higher-level summarizeText method. This encapsulates the endpoint logic and includes the mock data fallback, which improves robustness and aligns with the pattern used elsewhere in the SAMOAPIClient.
| const data = await apiClient.makeRequest(apiClient.endpoints.SUMMARIZE, { text: text }, 'POST'); | |
| const data = await apiClient.summarizeText(text); |
| // Make functions globally available | ||
| window.generateSampleText = generateSampleText; | ||
| window.processText = processText; | ||
| window.testWithRealAPI = testWithRealAPI; | ||
| window.callSummarizationAPI = callSummarizationAPI; | ||
| window.updateElement = updateElement; | ||
| window.showResultsSections = showResultsSections; | ||
| window.addToProgressConsole = addToProgressConsole; | ||
| window.clearProgressConsole = clearProgressConsole; | ||
| window.createEmotionChart = createEmotionChart; | ||
| window.resetToInputScreen = resetToInputScreen; | ||
| window.clearAllResultContent = clearAllResultContent; | ||
| window.manageApiKey = manageApiKey; | ||
| window.clearAll = clearAllResultContent; // Alias for clearAll function |
There was a problem hiding this comment.
Assigning multiple functions to the window object pollutes the global namespace and can lead to conflicts. Since demo-initialization.js already attaches event listeners to the UI elements, these global assignments are likely redundant. It's better to rely on the event listeners and remove these assignments. If some functions must be global, consider namespacing them under a single object, e.g., window.SAMODemo = { ... }.
| OPENAI_PROXY: '/proxy/openai' | ||
| }; | ||
|
|
||
| console.log('🔧 Running in localhost development mode - using deployed Cloud Run API'); |
There was a problem hiding this comment.
The log message is misleading. It states "using deployed Cloud Run API" but the BASE_URL is set to http://localhost:8002. This can cause confusion during development. The message should accurately reflect that a local API server is being used.
| console.log('🔧 Running in localhost development mode - using deployed Cloud Run API'); | |
| console.log('🔧 Running in localhost development mode - using local API server at http://localhost:8002'); |
| processBtn.addEventListener('click', function() { | ||
| console.log('🔘 Process button clicked!'); | ||
| // Use enhanced state management processing | ||
| if (typeof processTextWithStateManagement === 'function') { | ||
| processTextWithStateManagement(); | ||
| } else if (typeof window.processText === 'function') { | ||
| // Fallback to original function | ||
| if (window.LayoutManager?.showProcessingState) { | ||
| window.LayoutManager.showProcessingState(); | ||
| } | ||
| window.processText(true); // Skip state check since showProcessingState() handles it | ||
| } else { | ||
| console.error('❌ processText function not available'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The event listener for the process button contains complex fallback logic, checking for processTextWithStateManagement before falling back to window.processText. This indicates a fragile integration between the modules. To simplify and improve robustness, you should ensure processTextWithStateManagement is always available and remove the fallback, making it the single entry point for this action. A similar simplification should be applied to the 'Clear' button listener.
| if (maybe && typeof maybe.then === 'function') { | ||
| maybe.then(onDone).catch((error) => { | ||
| console.error('Processing error:', error); | ||
| LayoutManager.resetToInitialState(); | ||
| }); | ||
| } else { | ||
| onDone(); | ||
| } |
There was a problem hiding this comment.
The check if (maybe && typeof maybe.then === 'function') is redundant. Since processText is an async function, it will always return a Promise. You can directly use .then() and .catch() on its return value, and the else block is unreachable.
maybe.then(onDone).catch((error) => {
console.error('Processing error:', error);
LayoutManager.resetToInitialState();
});| showMessage(message, type = 'info') { | ||
| // Create a simple toast notification | ||
| const toast = document.createElement('div'); | ||
| toast.className = `toast-notification toast-${type}`; | ||
| toast.textContent = message; | ||
| toast.style.cssText = ` | ||
| position: fixed; | ||
| top: 20px; | ||
| right: 20px; | ||
| padding: 12px 20px; | ||
| border-radius: 6px; | ||
| color: white; | ||
| font-weight: 500; | ||
| z-index: 10000; | ||
| opacity: 0; | ||
| transition: opacity 0.3s ease; | ||
| `; | ||
|
|
||
| // Set background color based on type | ||
| const colors = { | ||
| success: '#28a745', | ||
| error: '#dc3545', | ||
| info: '#17a2b8' | ||
| }; | ||
| toast.style.backgroundColor = colors[type] || colors.info; | ||
|
|
||
| document.body.appendChild(toast); | ||
|
|
||
| // Animate in | ||
| setTimeout(() => toast.style.opacity = '1', 100); | ||
|
|
||
| // Remove after delay | ||
| setTimeout(() => { | ||
| toast.style.opacity = '0'; | ||
| setTimeout(() => document.body.removeChild(toast), 300); | ||
| }, 3000); | ||
| } |
There was a problem hiding this comment.
This file implements a custom showMessage function to display toast notifications. However, comprehensive-demo.js already defines showInlineMessage for a similar purpose. Having duplicated UI notification logic makes the code harder to maintain and can lead to an inconsistent user experience. This logic should be consolidated into a single, shared utility, perhaps within layout-manager.js or its own file, that all modules can use.
- Replace HTML redirect content in favicon.ico with proper favicon file - Move hardcoded production URLs to use centralized config.js configuration - Add config.js script tags to load configuration before other scripts - Update API_BASE_URL references to use window.SAMO_CONFIG - Improve maintainability by centralizing API endpoint configuration This resolves favicon display issues and eliminates hardcoded URLs for better environment management and deployment flexibility.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
website/favicon.icois excluded by!**/*.icowebsite/favicon.svgis excluded by!**/*.svg
📒 Files selected for processing (7)
website/comprehensive-demo.html(1 hunks)website/index.html(4 hunks)website/js/comprehensive-demo.js(1 hunks)website/js/config.js(1 hunks)website/js/demo-initialization.js(1 hunks)website/js/layout-manager.js(1 hunks)website/js/voice-recorder.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (5)
website/js/demo-initialization.js (1)
117-124: Guard Bootstrap tooltip initializationLine 120 references the global
bootstrapsymbol without checking it exists; if the CDN fails or Bootstrap loads after this script, you get a ReferenceError and the whole initialization short-circuits. Please verifywindow.bootstrap?.Tooltipbefore instantiating tooltips (logging a warning otherwise).- const tooltipTriggerList = [].slice.call(document.querySelectorAll('[data-bs-toggle="tooltip"]')); - const tooltipList = tooltipTriggerList.map(function (tooltipTriggerEl) { - return new bootstrap.Tooltip(tooltipTriggerEl); - }); + let tooltipList = []; + if (window.bootstrap?.Tooltip) { + const tooltipTriggerList = [].slice.call(document.querySelectorAll('[data-bs-toggle="tooltip"]')); + tooltipList = tooltipTriggerList.map((tooltipTriggerEl) => new window.bootstrap.Tooltip(tooltipTriggerEl)); + } else { + console.warn('⚠️ Bootstrap library not detected. Tooltips were not initialized.'); + }website/js/config.js (1)
80-80: Correct the localhost log messageLine 80 says “using deployed Cloud Run API” even though the override just pointed
BASE_URLathttp://localhost:8002. That misleads anyone debugging environment routing—please update the log string to match the actual destination.- console.log('🔧 Running in localhost development mode - using deployed Cloud Run API'); + console.log('🔧 Running in localhost development mode - using local API server at http://localhost:8002');website/js/voice-recorder.js (1)
176-189: Handle transcribeAudio return value correctly
SAMOAPIClient.transcribeAudioalready returns the parsed JSON frommakeRequest; it is not a rawResponsewithok/json()helpers. Treating it as aResponsemakes the happy path throw (“response.ok is undefined”), so transcription never reaches the UI. Please use the returned object directly.- const response = await apiClient.transcribeAudio(audioFile); - - if (response.ok) { - const result = await response.json(); - console.log('✅ Transcription successful:', result); - - // Display results in the UI - this.displayTranscriptionResults(result); - } else { - const errorText = await response.text().catch(() => 'Unknown error'); - throw new Error(`API request failed (${response.status}): ${errorText}`); - } + const result = await apiClient.transcribeAudio(audioFile); + console.log('✅ Transcription successful:', result); + + this.displayTranscriptionResults(result);website/js/comprehensive-demo.js (2)
768-770: Re-use window.apiClient + detectEmotions fallbackEchoing the earlier feedback: instantiating a fresh
SAMOAPIClienthere resets the cold-start optimisation every run and, more importantly, callingmakeRequestdirectly skips the resiliency logic indetectEmotions(mock fallback, friendly errors). When the API 429s/5xxs, the demo now hard fails instead of degrading gracefully. Please reuse the global client and call the high-level helper so the built-in safeguards stay active.- const apiClient = new SAMOAPIClient(); - const data = await apiClient.makeRequest(apiClient.endpoints.EMOTION, { text: testText }, 'POST'); + const apiClient = window.apiClient ?? (window.apiClient = new SAMOAPIClient()); + const data = await apiClient.detectEmotions(testText);
872-878: Leverage the global client + summarizeText fallbackSame underlying issue as above: this reinvents the API client each call and bypasses
summarizeText, so cold-start tracking and the mock-summary fallback never run. Any transient failure now blocks the user. Please reusewindow.apiClient(initialised on DOM ready) and delegate tosummarizeTextto retain the error-handling contract.- const apiClient = new SAMOAPIClient(); - const data = await apiClient.makeRequest(apiClient.endpoints.SUMMARIZE, { text: text }, 'POST'); + const apiClient = window.apiClient ?? (window.apiClient = new SAMOAPIClient()); + const data = await apiClient.summarizeText(text);
| const urlParams = new URLSearchParams(window.location.search); | ||
| if (urlParams.get('showDebug') === '1') { | ||
| LayoutManager.toggleDebugSection(true); | ||
| } | ||
|
|
There was a problem hiding this comment.
Guard LayoutManager before toggling debug section
Line 104 assumes LayoutManager is always defined; if the module fails to load, this throws a ReferenceError as soon as ?showDebug=1 is used, aborting the entire initializer. Please null-check via window.LayoutManager?.toggleDebugSection(...) (and log a warning when missing) before calling.
- LayoutManager.toggleDebugSection(true);
+ if (window.LayoutManager?.toggleDebugSection) {
+ window.LayoutManager.toggleDebugSection(true);
+ } else {
+ console.warn('⚠️ LayoutManager not available; debug section remains hidden.');
+ }📝 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.
| const urlParams = new URLSearchParams(window.location.search); | |
| if (urlParams.get('showDebug') === '1') { | |
| LayoutManager.toggleDebugSection(true); | |
| } | |
| const urlParams = new URLSearchParams(window.location.search); | |
| if (urlParams.get('showDebug') === '1') { | |
| if (window.LayoutManager?.toggleDebugSection) { | |
| window.LayoutManager.toggleDebugSection(true); | |
| } else { | |
| console.warn('⚠️ LayoutManager not available; debug section remains hidden.'); | |
| } | |
| } |
🤖 Prompt for AI Agents
In website/js/demo-initialization.js around lines 102 to 106, the code calls
LayoutManager.toggleDebugSection(true) without ensuring LayoutManager exists
which can throw a ReferenceError if the module failed to load; update the call
to guard via window.LayoutManager?.toggleDebugSection(true) (or check if
window.LayoutManager and typeof toggleDebugSection === 'function') and if
missing emit a console.warn explaining that LayoutManager is unavailable so
initialization can continue without throwing.
| emergencyReset() { | ||
| console.warn('🚨 Emergency reset: processing state appears stuck, forcing reset...'); | ||
| this.isProcessing = false; | ||
| this.activeRequests.clear(); | ||
| this.currentState = 'initial'; | ||
| // Also clear any UI elements that might be stuck | ||
| if (typeof clearAllResultContent === 'function') { | ||
| clearAllResultContent(); | ||
| } | ||
| }, |
There was a problem hiding this comment.
Emergency reset must abort outstanding requests
emergencyReset() currently drops the references in activeRequests without aborting the controllers, so those fetches continue running and can mutate the UI after we've “reset” the layout. This is easy to trigger because showProcessingState() invokes emergencyReset() on any guarded re-entry. Please cancel the tracked requests (and clear the timestamp) before wiping the state.
emergencyReset() {
console.warn('🚨 Emergency reset: processing state appears stuck, forcing reset...');
- this.isProcessing = false;
- this.activeRequests.clear();
- this.currentState = 'initial';
+ this.cancelActiveRequests();
+ this.isProcessing = false;
+ this.processingStartTime = null;
+ this.currentState = 'initial';
// Also clear any UI elements that might be stuck
if (typeof clearAllResultContent === 'function') {
clearAllResultContent();
}
},📝 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.
| emergencyReset() { | |
| console.warn('🚨 Emergency reset: processing state appears stuck, forcing reset...'); | |
| this.isProcessing = false; | |
| this.activeRequests.clear(); | |
| this.currentState = 'initial'; | |
| // Also clear any UI elements that might be stuck | |
| if (typeof clearAllResultContent === 'function') { | |
| clearAllResultContent(); | |
| } | |
| }, | |
| emergencyReset() { | |
| console.warn('🚨 Emergency reset: processing state appears stuck, forcing reset...'); | |
| this.cancelActiveRequests(); | |
| this.isProcessing = false; | |
| this.processingStartTime = null; | |
| this.currentState = 'initial'; | |
| // Also clear any UI elements that might be stuck | |
| if (typeof clearAllResultContent === 'function') { | |
| clearAllResultContent(); | |
| } | |
| }, |
🤖 Prompt for AI Agents
In website/js/layout-manager.js around lines 23 to 32, emergencyReset()
currently clears activeRequests without aborting ongoing fetches; update it to
first iterate the entries in this.activeRequests and for each entry: if it has a
controller property that is an AbortController (or the entry itself is an
AbortController) call controller.abort(); remove any timestamp property/field on
the entry (or set entry.timestamp = null) to prevent stale time-based logic;
only after aborting and clearing timestamps call this.activeRequests.clear(),
then proceed to set this.isProcessing = false and this.currentState = 'initial'
and clear UI. Ensure you null-check entries and swallow any exceptions from
abort to avoid throwing during reset.
| // Check if processing is allowed | ||
| if (!this.startProcessing()) { | ||
| console.warn('⚠️ Cannot start processing - operation already in progress'); | ||
| // Try emergency reset and retry once | ||
| console.warn('🔄 Attempting emergency reset and retry...'); | ||
| this.emergencyReset(); | ||
| if (!this.startProcessing()) { | ||
| console.error('❌ Emergency reset failed - processing still blocked'); | ||
| return false; | ||
| } | ||
| console.log('✅ Emergency reset successful - processing can proceed'); | ||
| } |
There was a problem hiding this comment.
Avoid emergency reset when a run is still active
When startProcessing() returns false, it means a run is legitimately in progress (and still within the 2‑minute window). Forcing emergencyReset() at that moment drops the guard while the original network requests keep running, so you can end up with overlapping jobs and stale callbacks racing the UI. Since startProcessing() already force-resets once the timeout is exceeded, this branch should just bail out.
// Check if processing is allowed
if (!this.startProcessing()) {
console.warn('⚠️ Cannot start processing - operation already in progress');
- // Try emergency reset and retry once
- console.warn('🔄 Attempting emergency reset and retry...');
- this.emergencyReset();
- if (!this.startProcessing()) {
- console.error('❌ Emergency reset failed - processing still blocked');
- return false;
- }
- console.log('✅ Emergency reset successful - processing can proceed');
+ return 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.
| // Check if processing is allowed | |
| if (!this.startProcessing()) { | |
| console.warn('⚠️ Cannot start processing - operation already in progress'); | |
| // Try emergency reset and retry once | |
| console.warn('🔄 Attempting emergency reset and retry...'); | |
| this.emergencyReset(); | |
| if (!this.startProcessing()) { | |
| console.error('❌ Emergency reset failed - processing still blocked'); | |
| return false; | |
| } | |
| console.log('✅ Emergency reset successful - processing can proceed'); | |
| } | |
| // Check if processing is allowed | |
| if (!this.startProcessing()) { | |
| console.warn('⚠️ Cannot start processing - operation already in progress'); | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In website/js/layout-manager.js around lines 110 to 121, the code attempts an
emergencyReset() and retry when startProcessing() returns false; this can drop
the guard while an active run is still within its 2-minute window and cause
overlapping jobs. Change the branch so that if startProcessing() returns false
you do not call emergencyReset() or retry — simply log/warn and return false
immediately, letting the existing timeout-based reset logic handle expired runs.
| // Remove existing state classes | ||
| stepElement.classList.remove('active', 'completed', 'error'); | ||
| stepIcon.classList.remove('pending', 'active', 'completed', 'error'); | ||
|
|
||
| // Add new state | ||
| stepElement.classList.add(state); | ||
| stepIcon.classList.add(state); |
There was a problem hiding this comment.
Resetting steps should remove previous state classes
updateProgressStep() never removes the pending class from stepElement, so when you mark a step as active or completed the element keeps both classes and the visual state is inconsistent. Drop pending alongside the other state classes when resetting.
- stepElement.classList.remove('active', 'completed', 'error');
+ stepElement.classList.remove('pending', 'active', 'completed', 'error');📝 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.
| // Remove existing state classes | |
| stepElement.classList.remove('active', 'completed', 'error'); | |
| stepIcon.classList.remove('pending', 'active', 'completed', 'error'); | |
| // Add new state | |
| stepElement.classList.add(state); | |
| stepIcon.classList.add(state); | |
| // Remove existing state classes | |
| stepElement.classList.remove('pending', 'active', 'completed', 'error'); | |
| stepIcon.classList.remove('pending', 'active', 'completed', 'error'); | |
| // Add new state | |
| stepElement.classList.add(state); | |
| stepIcon.classList.add(state); |
🤖 Prompt for AI Agents
In website/js/layout-manager.js around lines 291 to 297, the reset logic removes
state classes from stepElement but omits 'pending', causing stepElement to
retain 'pending' when switching to 'active' or 'completed'; update the remove
call to include 'pending' (i.e., remove 'pending', 'active', 'completed',
'error' from stepElement) so the element no longer keeps the prior pending class
before adding the new state.
| try { | ||
| // Show processing state | ||
| this.showProcessingState(); | ||
|
|
||
| // Create a File object from the blob | ||
| const audioFile = new File([audioBlob], 'recording.webm', { | ||
| type: audioBlob.type | ||
| }); | ||
|
|
||
| // Get or create API client | ||
| let apiClient = window.apiClient; | ||
| if (!apiClient) { | ||
| console.log('⚠️ Global API client not available, creating new instance...'); | ||
| try { | ||
| // Try to create a new SAMOAPIClient instance | ||
| if (typeof SAMOAPIClient !== 'undefined') { | ||
| apiClient = new SAMOAPIClient(); | ||
| console.log('✅ Created new API client instance'); | ||
| } else { | ||
| throw new Error('SAMOAPIClient class not available'); | ||
| } | ||
| } catch (createError) { | ||
| throw new Error(`Unable to create API client: ${createError.message}`); | ||
| } | ||
| } | ||
|
|
||
| // Use API client to transcribe | ||
| if (apiClient && typeof apiClient.transcribeAudio === 'function') { | ||
| console.log('🔄 Sending audio for transcription...'); | ||
| const response = await apiClient.transcribeAudio(audioFile); | ||
|
|
||
| if (response.ok) { | ||
| const result = await response.json(); | ||
| console.log('✅ Transcription successful:', result); | ||
|
|
||
| // Display results in the UI | ||
| this.displayTranscriptionResults(result); | ||
| } else { | ||
| const errorText = await response.text().catch(() => 'Unknown error'); | ||
| throw new Error(`API request failed (${response.status}): ${errorText}`); | ||
| } | ||
| } else { | ||
| throw new Error('API client transcribeAudio method not available'); | ||
| } | ||
|
|
||
| } catch (error) { | ||
| console.error('Failed to transcribe audio:', error); | ||
|
|
||
| // Provide specific error messages based on error type | ||
| let userMessage = 'Transcription failed'; | ||
| if (error.message.includes('API client not available')) { | ||
| userMessage = 'Voice service unavailable. Please refresh the page and try again.'; | ||
| } else if (error.message.includes('Failed to fetch') || error.message.includes('Network')) { | ||
| userMessage = 'Network error. Please check your connection and try again.'; | ||
| } else if (error.message.includes('timeout')) { | ||
| userMessage = 'Request timeout. Please try with a shorter recording.'; | ||
| } else if (error.message.includes('400')) { | ||
| userMessage = 'Invalid audio format. Please try recording again.'; | ||
| } else if (error.message.includes('500')) { | ||
| userMessage = 'Server error. Please try again in a moment.'; | ||
| } else { | ||
| userMessage = `Transcription failed: ${error.message}`; | ||
| } | ||
|
|
||
| this.showError(userMessage); | ||
|
|
||
| // Reset processing state on error | ||
| if (window.LayoutManager && window.LayoutManager.isProcessing) { | ||
| window.LayoutManager.endProcessing(); | ||
| console.log('🔧 Processing state reset due to transcription error'); | ||
| } | ||
| } finally { | ||
| this.hideProcessingState(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Respect LayoutManager’s “already processing” guard
showProcessingState() returns false when LayoutManager.canStartProcessing() says “busy”. Ignoring that and continuing into the API call bypasses the concurrency guard and still triggers hideProcessingState() at the end, leaving the UI in an inconsistent state. Bail out early when the guard fires and only call hideProcessingState() if we actually entered the processing state.
- async processRecordedAudio(audioBlob) {
- try {
- // Show processing state
- this.showProcessingState();
+ async processRecordedAudio(audioBlob) {
+ let startedProcessing = false;
+ try {
+ // Show processing state
+ const processingState = this.showProcessingState();
+ if (processingState === false) {
+ console.warn('⚠️ A processing operation is already running; skipping transcription request.');
+ return;
+ }
+ startedProcessing = true;
@@
- } finally {
- this.hideProcessingState();
+ } finally {
+ if (startedProcessing) {
+ this.hideProcessingState();
+ }
}
}📝 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.
| try { | |
| // Show processing state | |
| this.showProcessingState(); | |
| // Create a File object from the blob | |
| const audioFile = new File([audioBlob], 'recording.webm', { | |
| type: audioBlob.type | |
| }); | |
| // Get or create API client | |
| let apiClient = window.apiClient; | |
| if (!apiClient) { | |
| console.log('⚠️ Global API client not available, creating new instance...'); | |
| try { | |
| // Try to create a new SAMOAPIClient instance | |
| if (typeof SAMOAPIClient !== 'undefined') { | |
| apiClient = new SAMOAPIClient(); | |
| console.log('✅ Created new API client instance'); | |
| } else { | |
| throw new Error('SAMOAPIClient class not available'); | |
| } | |
| } catch (createError) { | |
| throw new Error(`Unable to create API client: ${createError.message}`); | |
| } | |
| } | |
| // Use API client to transcribe | |
| if (apiClient && typeof apiClient.transcribeAudio === 'function') { | |
| console.log('🔄 Sending audio for transcription...'); | |
| const response = await apiClient.transcribeAudio(audioFile); | |
| if (response.ok) { | |
| const result = await response.json(); | |
| console.log('✅ Transcription successful:', result); | |
| // Display results in the UI | |
| this.displayTranscriptionResults(result); | |
| } else { | |
| const errorText = await response.text().catch(() => 'Unknown error'); | |
| throw new Error(`API request failed (${response.status}): ${errorText}`); | |
| } | |
| } else { | |
| throw new Error('API client transcribeAudio method not available'); | |
| } | |
| } catch (error) { | |
| console.error('Failed to transcribe audio:', error); | |
| // Provide specific error messages based on error type | |
| let userMessage = 'Transcription failed'; | |
| if (error.message.includes('API client not available')) { | |
| userMessage = 'Voice service unavailable. Please refresh the page and try again.'; | |
| } else if (error.message.includes('Failed to fetch') || error.message.includes('Network')) { | |
| userMessage = 'Network error. Please check your connection and try again.'; | |
| } else if (error.message.includes('timeout')) { | |
| userMessage = 'Request timeout. Please try with a shorter recording.'; | |
| } else if (error.message.includes('400')) { | |
| userMessage = 'Invalid audio format. Please try recording again.'; | |
| } else if (error.message.includes('500')) { | |
| userMessage = 'Server error. Please try again in a moment.'; | |
| } else { | |
| userMessage = `Transcription failed: ${error.message}`; | |
| } | |
| this.showError(userMessage); | |
| // Reset processing state on error | |
| if (window.LayoutManager && window.LayoutManager.isProcessing) { | |
| window.LayoutManager.endProcessing(); | |
| console.log('🔧 Processing state reset due to transcription error'); | |
| } | |
| } finally { | |
| this.hideProcessingState(); | |
| } | |
| } | |
| async processRecordedAudio(audioBlob) { | |
| let startedProcessing = false; | |
| try { | |
| // Show processing state | |
| const processingState = this.showProcessingState(); | |
| if (processingState === false) { | |
| console.warn('⚠️ A processing operation is already running; skipping transcription request.'); | |
| return; | |
| } | |
| startedProcessing = true; | |
| // Create a File object from the blob | |
| const audioFile = new File([audioBlob], 'recording.webm', { | |
| type: audioBlob.type | |
| }); | |
| // Get or create API client | |
| let apiClient = window.apiClient; | |
| if (!apiClient) { | |
| console.log('⚠️ Global API client not available, creating new instance...'); | |
| try { | |
| // Try to create a new SAMOAPIClient instance | |
| if (typeof SAMOAPIClient !== 'undefined') { | |
| apiClient = new SAMOAPIClient(); | |
| console.log('✅ Created new API client instance'); | |
| } else { | |
| throw new Error('SAMOAPIClient class not available'); | |
| } | |
| } catch (createError) { | |
| throw new Error(`Unable to create API client: ${createError.message}`); | |
| } | |
| } | |
| // Use API client to transcribe | |
| if (apiClient && typeof apiClient.transcribeAudio === 'function') { | |
| console.log('🔄 Sending audio for transcription...'); | |
| const response = await apiClient.transcribeAudio(audioFile); | |
| if (response.ok) { | |
| const result = await response.json(); | |
| console.log('✅ Transcription successful:', result); | |
| // Display results in the UI | |
| this.displayTranscriptionResults(result); | |
| } else { | |
| const errorText = await response.text().catch(() => 'Unknown error'); | |
| throw new Error(`API request failed (${response.status}): ${errorText}`); | |
| } | |
| } else { | |
| throw new Error('API client transcribeAudio method not available'); | |
| } | |
| } catch (error) { | |
| console.error('Failed to transcribe audio:', error); | |
| // Provide specific error messages based on error type | |
| let userMessage = 'Transcription failed'; | |
| if (error.message.includes('API client not available')) { | |
| userMessage = 'Voice service unavailable. Please refresh the page and try again.'; | |
| } else if (error.message.includes('Failed to fetch') || error.message.includes('Network')) { | |
| userMessage = 'Network error. Please check your connection and try again.'; | |
| } else if (error.message.includes('timeout')) { | |
| userMessage = 'Request timeout. Please try with a shorter recording.'; | |
| } else if (error.message.includes('400')) { | |
| userMessage = 'Invalid audio format. Please try recording again.'; | |
| } else if (error.message.includes('500')) { | |
| userMessage = 'Server error. Please try again in a moment.'; | |
| } else { | |
| userMessage = `Transcription failed: ${error.message}`; | |
| } | |
| this.showError(userMessage); | |
| // Reset processing state on error | |
| if (window.LayoutManager && window.LayoutManager.isProcessing) { | |
| window.LayoutManager.endProcessing(); | |
| console.log('🔧 Processing state reset due to transcription error'); | |
| } | |
| } finally { | |
| if (startedProcessing) { | |
| this.hideProcessingState(); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In website/js/voice-recorder.js around lines 149 to 223, showProcessingState()
can return false when LayoutManager reports busy; the current code ignores that
and proceeds, causing hideProcessingState() to run later and leaving the UI
inconsistent. Modify the method to capture the return value (e.g., started =
this.showProcessingState()), immediately bail out (return) if started is false,
and only call hideProcessingState() and LayoutManager.endProcessing() in the
catch/finally blocks when started is true so the processing state is only
cleared when you actually entered it.
- Update comprehensive-demo.html with improved layout and functionality - Add favicon.svg for modern vector icon support - Update favicon.ico with enhanced branding - Enhance index.html with Priority 1 features showcase and improved accessibility Fortress-compliant PR #190-A (4 files) - HTML Updates & Configuration Part of fortress split from original PR #190 for better review compliance. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add config.js for centralized configuration management - Add layout-manager.js for responsive layout and UI state management - Add voice-recorder.js for advanced audio recording capabilities Core modules provide foundation for website interactivity and user experience. These modules are designed to be imported by integration scripts in subsequent PRs. Fortress-compliant PR #190-B (3 files) - Core JavaScript Modules Part of fortress split from original PR #190 for better review compliance. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Add comprehensive-demo.js for complete demo page functionality and user interactions - Add demo-initialization.js for application startup and module coordination These integration scripts bring together the core modules from PR #190-B to create the complete interactive website experience. Handles user workflows, API integration, and coordinated initialization of all website features. Fortress-compliant PR #190-C (2 files) - Integration & Initialization Final part of fortress split from original PR #190 for better review compliance. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
🏰 Fortress Strategy Implementation Complete This PR has been superseded by fortress-compliant split PRs for better review compliance and manageable code review process. Fortress Split SummaryOriginal PR #190 (9 files) has been strategically split into three fortress-compliant PRs: ✅ PR #200 (PR #190-A): HTML Updates & Configuration
✅ PR #201 (PR #190-B): Core JavaScript Modules
✅ PR #202 (PR #190-C): Integration & Initialization
Strategic Benefits
Next Steps
All original functionality preserved - just organized for better review compliance! 🤖 Generated with Claude Code |
…A) (#200) * feat: add enhanced HTML files and configuration for website - Update comprehensive-demo.html with improved layout and functionality - Add favicon.svg for modern vector icon support - Update favicon.ico with enhanced branding - Enhance index.html with Priority 1 features showcase and improved accessibility Fortress-compliant PR #190-A (4 files) - HTML Updates & Configuration Part of fortress split from original PR #190 for better review compliance. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: address Copilot AI and coderabbitai review comments - Create missing js/config.js with centralized API configuration - Add proper accessibility attributes to navigation elements - Add aria-hidden=true to decorative icons in buttons - Update placeholder URL from api.example.com to your-api-domain.com - Add error handling for missing config.js file in both HTML files - Fix invalid JavaScript template literal syntax in Python code snippet - Use proper Python f-string syntax instead of ${API_BASE_URL} All changes improve accessibility, configuration management, and code quality. * fix: pin apt-get package versions to resolve DOK-DL3008 - Pin ca-certificates=20230311 for Debian bookworm - Pin curl=7.88.1-10+deb12u5 for Debian bookworm - Ensures reproducible builds and prevents version drift - Resolves Docker linting rule DOK-DL3008 --------- Co-authored-by: Claude <noreply@anthropic.com>
…s PR #190-C) (#202) * feat: add integration and initialization scripts for website - Add comprehensive-demo.js for complete demo page functionality and user interactions - Add demo-initialization.js for application startup and module coordination These integration scripts bring together the core modules from PR #190-B to create the complete interactive website experience. Handles user workflows, API integration, and coordinated initialization of all website features. Fortress-compliant PR #190-C (2 files) - Integration & Initialization Final part of fortress split from original PR #190 for better review compliance. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: guard LayoutManager.toggleDebugSection call with optional chaining to prevent ReferenceError * fix: reuse global API client and use detectEmotions method for proper fallbacks and tracking * fix: prefer sessionStorage over localStorage for API key storage to improve security * refactor: improve code organization and error handling - Combine multiple DOMContentLoaded listeners into single listener for clarity - Remove redundant setTimeout loading messages (already handled by addToProgressConsole) - Use global API client instance instead of creating new instances - Namespace functions under window.SamoDemo to avoid global pollution - Add user-visible error alerts when core functions are unavailable * fix: address code quality nitpicks for better maintainability demo-initialization.js: - Reference optional globals via window for module safety - Avoid enumerating window object for cleaner logging - Make initialization idempotent to prevent duplicate listeners - Remove unused tooltipList variable declaration comprehensive-demo.js: - Reuse global API client for summarization calls - Broaden network error detection to include 'Failed to fetch' - Add Accept header to all API requests - Remove unused summaryLength variable and inline calculation - Add updateApiKeyButtonStatus helper function - Prevent overwriting existing global API client * fix: add missing script imports for comprehensive-demo.js and demo-initialization.js --------- Co-authored-by: Claude <noreply@anthropic.com>
Summary
• Fortress-compliant implementation of interactive JavaScript modules from PR #169
• Part 2/2 of sophisticated website files from feat/clean-demo-website
• Enables full interactive emotion detection demo with 28 emotions
Changes (5/5 files - Fortress Compliant ✅)
• comprehensive-demo.js (58KB): Core demo functionality with DeBERTa v3 Large integration
• config.js: Configuration management and API endpoints
• demo-initialization.js: Demo setup and initialization logic
• layout-manager.js: UI layout and responsive design management
• voice-recorder.js: Voice recording and transcription capabilities
Technical Details
• Interactive Features: Voice recording, real-time emotion analysis, result visualization
• AI Integration: DeBERTa v3 Large model for 28-emotion classification
• Modern JavaScript: ES6+ modules with clean separation of concerns
• Responsive Design: Mobile-friendly layout management
• Error Handling: Comprehensive error handling and user feedback
Original Work Attribution
All files sourced from PR #169
feat/clean-demo-websitebranch.This PR represents the sophisticated interactive functionality that complements the core website files from PR #189.
Testing
• ✅ All files successfully copied from origin/feat/clean-demo-website
• ✅ Fortress compliance verified (5/5 files)
• ✅ Pre-commit hooks passed
• ✅ Local server integration ready
🤖 Generated with Claude Code
Summary by Sourcery
Implement interactive JavaScript modules for the demo website, enabling voice recording, emotion detection, and summarization with centralized configuration and robust UI state management
New Features:
Enhancements:
Summary by CodeRabbit