Skip to content

Sentiment module#24

Open
Azurinine wants to merge 11 commits intomainfrom
sentiment-module
Open

Sentiment module#24
Azurinine wants to merge 11 commits intomainfrom
sentiment-module

Conversation

@Azurinine
Copy link
Contributor

Added sentiment analysis. Currently a dummy button added to switch between aggregate and per sentence, added loading animatinos.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an initial “Sentiment” module that calls a new server endpoint backed by Hugging Face inference, plus UI state for switching between aggregate vs per-sentence charting and new loading/empty states.

Changes:

  • Add /api/sentiment server endpoint using @huggingface/inference and .env-based configuration.
  • Add ApiClient.getSentiment() and a Sentiment module that fetches + charts results with loading/empty states.
  • Add a global UI toggle (isAggregate) and related styling.

Reviewed changes

Copilot reviewed 9 out of 11 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
src/services/apiClient.ts Adds getSentiment() POST client method.
src/components/modules/Sentiment.tsx New sentiment fetching, aggregation/per-sentence chart shaping, and loading/empty UI.
src/components/modules/Sentiment.css Adds styles for loading and empty states.
src/components/Workspace.tsx Wires conversation messages + aggregate mode into Sentiment module.
src/App.tsx Adds isAggregate state and a toggle button in the sidebar.
src/App.css Sidebar layout adjustments + toggle button styling.
server/index.ts Loads .env, initializes Hugging Face client, adds /api/sentiment endpoint.
package.json Adds HF inference + dotenv + loader spinner deps.
package-lock.json Lockfile updates for added dependencies.
.gitignore Ignores .env.
.env.example Documents HUGGING_FACE_TOKEN configuration.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +110 to +128
useEffect(() => {
setIsLoading(true);
const controller = new AbortController();
const studentContents = sharedMessages
.filter((msg) => msg.role === "STUDENT")
.map((msg) => msg.content);
if (studentContents.length === 0) {
setSentimentResults(null);
setIsLoading(false);
return;
}
fetchSentiment(studentContents, controller.signal)
.then((results) => {
setSentimentResults(results.length > 0 ? results : null);
})
.catch((err) => {
if (err?.name !== "AbortError") console.error(err);
})
.finally(() => setIsLoading(false));
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a new fetch, sentimentResults is not cleared, and in the error path it is left unchanged. If the API fails (including 503 when Hugging Face token is missing), the UI can fall back to showing stale results from a previous conversation. Consider clearing sentimentResults when starting the effect and/or setting it to null in the catch branch, and optionally rendering a dedicated error state instead of the "Select a Conversation" empty state.

Copilot uses AI. Check for mistakes.
Comment on lines +9 to +15
interface FileMessageRow {
id: number;
role: string | null;
content: string;
timestamp: string | null;
sortOrder: number;
}
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FileMessageRow duplicates the FileMessage shape that already exists in shared/types (and is already used in Workspace). Reusing the shared type avoids drift if the backend message schema changes.

Copilot uses AI. Check for mistakes.
Comment on lines +306 to +317
const { sentences } = req.body as { sentences?: string[] };
if (!Array.isArray(sentences) || sentences.length === 0) {
return res.status(400).json({ error: 'Request body must include a non-empty array: sentences' });
}
const SENTIMENT_MODEL = 'j-hartmann/emotion-english-distilroberta-base';
// Model max length is 512 tokens; ~4 chars/token → truncate to stay under
const MAX_INPUT_CHARS = 500;
const truncate = (s: string) =>
s.length <= MAX_INPUT_CHARS ? s : s.slice(0, MAX_INPUT_CHARS).trim();
const results = await Promise.all(
sentences.map(async (sentence: string) => {
const input = truncate(sentence);
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sentences is only validated as an array, but individual elements are not validated. If a client sends non-strings, truncate(sentence) will throw (e.g., length undefined) and return a 500. Validate each element is a string (and possibly non-empty after trimming) and return a 400 for invalid input.

Copilot uses AI. Check for mistakes.
Comment on lines +315 to +324
const results = await Promise.all(
sentences.map(async (sentence: string) => {
const input = truncate(sentence);
const output = await hf.textClassification({
model: SENTIMENT_MODEL,
inputs: input,
});
return { sentence, sentiment: output };
})
);
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This endpoint fan-outs one Hugging Face request per sentence via Promise.all(...) with no upper bound. A large sentences array can create very high concurrency, trigger rate limiting, and incur unexpected cost/latency (and can be abused for DoS). Consider enforcing a maximum number of sentences per request and using a concurrency limiter / batching strategy instead of unbounded Promise.all.

Copilot uses AI. Check for mistakes.
setIsLoading(true);
const controller = new AbortController();
const studentContents = sharedMessages
.filter((msg) => msg.role === "STUDENT")
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sentiment fetch filters messages with msg.role === "STUDENT", but elsewhere in the app roles are normalized to lowercase values like student, student: (see Workspace message parsing and Messages module). As-is, this will likely send an empty sentences list and never fetch sentiment. Normalize the role the same way (e.g., msg.role?.toLowerCase() with the same variants) before filtering.

Suggested change
.filter((msg) => msg.role === "STUDENT")
.filter((msg) => {
const normalizedRole = msg.role?.toLowerCase().replace(/:$/, "");
return normalizedRole === "student";
})

Copilot uses AI. Check for mistakes.
Comment on lines +17 to +48
interface SentimentProps {
onClose?: () => void;
onResize?: (newColSpan: number, newRowSpan: number) => void;
colSpan?: number;
rowSpan?: number;
data?: SentimentSeries[];
sharedMessages: FileMessageRow[];
RAW_SCORE_THREHOLD?: number;
isAggregate: boolean;
}

type SentimentData = {category: string, value: number};
type SentimentData = { category: string; value: number; sentence?: string };

type SentimentSeries = {
label: string;
data: SentimentData[];
};

const defaultData: SentimentSeries[] =
[
{
label: 'Default Sentiment',
data: [
{category: 'Angry', value: 1},
{category: 'Happy', value: 5},
{category: 'Neutral', value: 7}
]
}
]
type SentimentResult = {
sentence: string;
sentiment: Array<{ label: string; score: number }>;
};

export function Sentiment({
onClose,
onResize,
colSpan = 2,
rowSpan = 1,
data = defaultData,
data,
sharedMessages,
RAW_SCORE_THREHOLD = 0.5,
isAggregate = false,
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RAW_SCORE_THREHOLD is misspelled ("THREHOLD"). Since this is part of the public props for the module, it will propagate typos to all callers and makes code harder to search/maintain. Rename it to RAW_SCORE_THRESHOLD (and update usages) before this API spreads.

Copilot uses AI. Check for mistakes.
Comment on lines +105 to +130
const fetchSentiment = async (sentences: string[], signal?: AbortSignal) => {
if (sentences.length === 0) return [];
return apiClient.getSentiment(sentences, signal);
};

useEffect(() => {
setIsLoading(true);
const controller = new AbortController();
const studentContents = sharedMessages
.filter((msg) => msg.role === "STUDENT")
.map((msg) => msg.content);
if (studentContents.length === 0) {
setSentimentResults(null);
setIsLoading(false);
return;
}
fetchSentiment(studentContents, controller.signal)
.then((results) => {
setSentimentResults(results.length > 0 ? results : null);
})
.catch((err) => {
if (err?.name !== "AbortError") console.error(err);
})
.finally(() => setIsLoading(false));
return () => controller.abort();
}, [sharedMessages]);
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fetchSentiment is defined inline and then referenced inside useEffect, but it is not included in the dependency array. This can trigger react-hooks/exhaustive-deps warnings and makes the effect harder to reason about. Either inline the call to apiClient.getSentiment in the effect, or wrap fetchSentiment in useCallback and include it in the deps.

Copilot uses AI. Check for mistakes.
signal,
});
if (!response.ok) {
throw new Error('Failed to fetch sentiment');
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error thrown here is very generic (Failed to fetch sentiment) and drops useful debugging info like status code / response body. Consider including response.status (and optionally await response.text() for non-2xx) similar to the other ApiClient methods, so callers can distinguish 400 vs 503 vs 500.

Suggested change
throw new Error('Failed to fetch sentiment');
let errorBody: string | undefined;
try {
errorBody = await response.text();
} catch {
// Ignore errors while reading error body
}
const message = `Failed to fetch sentiment (status ${response.status}` +
(errorBody ? `, body: ${errorBody}` : '') +
')';
throw new Error(message);

Copilot uses AI. Check for mistakes.
src/App.tsx Outdated
onFileUpload={refreshFiles}
onFileDelete={refreshFiles}
/>
<button className="sentiment-chart-button" onClick={() => setIsAggregate(!isAggregate)}>
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a toggle button, but it doesn’t expose its state to assistive technologies. Consider adding aria-pressed={isAggregate} (or switching to a checkbox/switch control) so screen readers can announce whether Aggregate or Per sentence mode is active.

Suggested change
<button className="sentiment-chart-button" onClick={() => setIsAggregate(!isAggregate)}>
<button
className="sentiment-chart-button"
onClick={() => setIsAggregate(!isAggregate)}
aria-pressed={isAggregate}
>

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@m1nce m1nce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there seems to be a bug with cse8a files, where the sentiment doesn't display for them. other than that, works well for a majority of cases within dsc10

Copy link
Contributor

@m1nce m1nce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very very good work 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants