[← Back to Reviews Index](../Stewards%20Reviews%20Index.md)

# React State & Performance Practices Review — Azure-AI-RAG-CSharp-Semantic-Kernel-Functions

| Field | Value |
|---|---|
| Project | Azure-AI-RAG-CSharp-Semantic-Kernel-Functions |
| Run Date | 2026-03-21 |
| Steward | React SP Practices Steward (RSPA) |
| Critical | 2 |
| Notable | 4 |
| Minor | 3 |
| Info | 2 |
| Total | 11 |

---

## 1. Component Architecture Overview

The React frontend (`src/web`) is a small Create React App project with five source components:

| Component | File | Type | Purpose |
|---|---|---|---|
| `App` | `src/App.js` | Functional | Router shell |
| `Main` (exported as `App`) | `src/Main.js` | Class component | Page layout / orchestrator |
| `SupportAgent` | `src/SupportAgent/Agent.js` | Functional (hooks) | Chat UI with state management |
| `ChatLayout` | `src/SupportAgent/ChatLayout.js` | Functional (pure-ish) | Message list renderer |
| `AzureFreeAccount` | `src/AzureFreAccount.js` | Functional | Static promo banner |
| `AzureServiceCards` | `src/AzureServiceCards.js` | Functional | Static service-card grid |

The application has no TypeScript, no global state library, and no dedicated state management beyond `useState` in `SupportAgent`. The codebase is intentionally minimal (demo/sample), but several correctness and best-practice issues are present.

---

## 2. State Management Assessment

`SupportAgent` is the only stateful component. It holds three state variables:

- `session` (string) — tracks the session ID returned by the backend.
- `chatPrompt` (string) — tracks the controlled text-field value.
- `message` (array of objects) — accumulates all chat messages.

**Observations:**
- State count is appropriate for a component of this responsibility; `useReducer` is not needed yet.
- `Main.js` is a class component that declares `this.state = { question: '', searchResults: [] }` but never reads or mutates that state anywhere in the component. The dead state adds confusion and is a maintenance liability.
- `SupportAgent` has no error state. If either fetch call fails the UI silently stalls — the user sees no indication that the session or chat request failed.
- `chatPrompt` is initialised with a hard-coded example question (`'How do I fix Access Denied...'`) but the `<TextField>` also carries a `defaultValue` prop set to the same string. For a controlled input `defaultValue` is ignored by React; only `value` is used. This is harmless but misleading.

---

## 3. useEffect Correctness Assessment

`SupportAgent` contains a single `useEffect`:

```js
useEffect(() => {
  if (session === '') handleSession()
}, [])
```

**Issue 1 — Missing dependency (`session`):** The effect reads `session` from component scope but the dependency array is empty (`[]`). The ESLint `react-hooks/exhaustive-deps` rule would flag this. At runtime the stale-closure risk is low because the guard `session === ''` is evaluated once on mount; however, the declared intent and the actual dependencies are misaligned.

**Issue 2 — No fetch cancellation:** `handleSession` fires a `fetch` call. If the component unmounts before the request resolves (e.g., fast navigation), the `.then` callback still runs and calls `setSession` on an unmounted component. React 18 suppresses the `setState on unmounted component` warning, but the update still executes against stale closure state. An `AbortController` should cancel the request on cleanup.

**Issue 3 — `handlePrompt` fetch not cancelled:** Similarly, `handlePrompt` fires a `fetch` against `/chat` with no cancellation mechanism. If the user submits quickly and navigates away, the response `.then` fires on unmounted component state.

**Issue 4 — No error handling on fetches:** Both `fetch` calls use `.then()` chains with no `.catch()`. Network errors or non-2xx responses will result in unhandled promise rejections. `response.json()` will throw if the response body is not valid JSON, making `setMessage` with `res.resp` produce `undefined` in the chat bubble.

---

## 4. Memoization Assessment

Given the small size of the application, aggressive memoization is not required. However:

- `ChatLayout` renders the full message list on every parent re-render. It is not wrapped in `React.memo`. For a chat app with a growing message list this will become expensive as every keystroke in the `TextField` causes a full re-render of `SupportAgent` and therefore `ChatLayout`.
- No `useCallback` or `useMemo` usage exists anywhere. At current scale this is acceptable, but `handlePrompt` and `handleSession` are recreated on every render and passed inline; if `ChatLayout` were memoized, these would need `useCallback`.
- No expensive inline computations identified that require `useMemo`.

---

## 5. Key Prop Assessment

`ChatLayout.js` renders a list of message objects:

```js
{messages.messages.map((obj, i = 0) => (
  <div className="bubbleContainer" key={i}>
    <Box key={i++} ...>
```

**Critical issues found:**

1. **Array index used as key** — `i` starts at `0` and increments. When React reconciles a list using index-based keys, inserting or removing messages at any position other than the end causes React to remount existing nodes unnecessarily, losing DOM state.

2. **Index increment side-effect inside JSX (`i++`)** — The expression `i++` mutates the loop variable inside JSX. This is an anti-pattern: the `key` on `Box` starts at the same value as the enclosing `div` (because `i = 0` in the parameter default resets it for each invocation — this is actually a bug in the parameter default; `i` is the map callback's second argument, so it behaves correctly as the index, but the `i++` on `Box` means the inner key is always `index + 1`). The result is mismatched keys between sibling elements and potential reconciliation errors.

3. **Duplicate keys** — `div.bubbleContainer` and the inner `Box` both receive a `key` with effectively sequential but offset values derived from the same counter. React keys must be unique among siblings at the same level, not across levels, but using `i` and `i++` here is still confusing and brittle.

---

## 6. Component Patterns Assessment

### Class component in a hooks-based app

`Main.js` exports a class component. The rest of the app is functional. This is a mixing of paradigms. The class component holds no non-trivial lifecycle logic that would justify keeping it as a class; it could be a plain functional component.

### Dead/unused state in `Main.js`

```js
this.state = { question: '', searchResults: [] }
```

`question` and `searchResults` are set in the constructor but never read or updated. They should be removed.

### `Main.js` naming collision

`Main.js` exports a class named `App`, but `App.js` also exports a function named `App`. Both are in scope simultaneously if both are imported. The class is imported as `Main` in `App.js`, so there is no runtime collision, but the class name `App` inside `Main.js` is misleading and can confuse tooling and developers.

### `ChatLayout` destructuring

`ChatLayout` receives `messages` as its props object but accesses `messages.messages` (the prop named `messages` within the props object). The function signature is `function ChatLayout(messages)` — the entire props object is named `messages`. Idiomatic React destructures props: `function ChatLayout({ messages })`. The current signature forces the `.messages.messages` double-dot access and is error-prone.

### `console.log` in production code

`Agent.js` line 38: `console.log(res)` — left in from development. This leaks API response content to the browser console in production.

### No TypeScript / all plain JavaScript

The project has no TypeScript. All component props are untyped. See TypeScript Assessment section.

### Component sizes

No component exceeds 100 lines; size is not an issue.

---

## 7. TypeScript Assessment

The project is JavaScript only — there is no TypeScript configuration and no `.ts` / `.tsx` files. This means:

- All component props are untyped.
- The shape of API response objects (`res.resp`, `res.session_id`) is undocumented in code.
- There is no prop validation (no PropTypes, no TS interfaces).
- Misuse of props (e.g., `ChatLayout(messages)` vs `ChatLayout({ messages })`) would be caught immediately by TypeScript.

No `any`, `@ts-ignore`, or `@ts-expect-error` are present (the project has no TypeScript at all), so TypeScript-specific anti-patterns are absent by default. However, the absence of TypeScript itself is a notable quality gap for a production-bound application.

---

## 8. Findings

| Severity | ID | Title | File |
|---|---|---|---|
| 🔴 Critical | RSPA-EFFECT-001 | useEffect fetch calls lack AbortController cleanup | `src/SupportAgent/Agent.js` |
| 🔴 Critical | RSPA-KEY-001 | Index used as key with in-JSX mutation (`i++`) in message list | `src/SupportAgent/ChatLayout.js` |
| 🟡 Notable | RSPA-EFFECT-002 | useEffect dependency array missing `session` | `src/SupportAgent/Agent.js` |
| 🟡 Notable | RSPA-STATE-001 | Dead state (`question`, `searchResults`) declared in Main class but never used | `src/Main.js` |
| 🟡 Notable | RSPA-COMP-001 | Props object not destructured in ChatLayout — forces `messages.messages` double-access | `src/SupportAgent/ChatLayout.js` |
| 🟡 Notable | RSPA-EFFECT-003 | No error handling on fetch calls — unhandled rejections and silent failures | `src/SupportAgent/Agent.js` |
| 🟢 Minor | RSPA-COMP-002 | Class component in a hooks-based app with misleading exported name `App` | `src/Main.js` |
| 🟢 Minor | RSPA-COMP-003 | `console.log(res)` left in production code | `src/SupportAgent/Agent.js` |
| 🟢 Minor | RSPA-TS-001 | No TypeScript — all component props are untyped | `src/web` |
| ℹ️ Info | RSPA-MEMO-001 | ChatLayout not memoized — will re-render on every keystroke at scale | `src/SupportAgent/ChatLayout.js` |
| ℹ️ Info | RSPA-STATE-002 | `defaultValue` on controlled TextField is redundant — `value` prop takes precedence | `src/SupportAgent/Agent.js` |

---

### Finding Details

#### RSPA-EFFECT-001 — Critical: useEffect fetch calls lack AbortController cleanup

Both `handleSession` (called from `useEffect`) and `handlePrompt` (called on button click) fire `fetch` requests with no cleanup. When the `SupportAgent` component unmounts before a response arrives, the `.then` callbacks still execute and attempt to call `setSession` or `setMessage` on a potentially stale state reference. For the session fetch specifically, it is triggered inside a `useEffect` that has no cleanup return, guaranteeing this path can be hit on fast navigations.

**Recommendation:** Introduce an `AbortController` in `handleSession` and return a cleanup function from the `useEffect` that calls `controller.abort()`. For `handlePrompt`, track an abort reference and cancel on unmount, or use a `useRef` flag that is set to `false` on cleanup.

---

#### RSPA-KEY-001 — Critical: Index used as key with in-JSX mutation in message list

`ChatLayout.js` line 9-12: the map callback parameter is declared as `(obj, i = 0)` where `i = 0` is a default parameter (not a reset — `i` is still the map index). Both `<div>` and `<Box>` receive key props derived from `i` and `i++` respectively. This results in:

- Index-based keys — React cannot distinguish between message reordering and content change.
- The inner `Box` always gets a key that is one higher than the outer `div`, producing offset keys that change meaning as messages accumulate.
- Mutation of a loop variable inside JSX is an anti-pattern; it makes render output hard to reason about.

**Recommendation:** Add a stable `id` field to each message object (e.g., a UUID or incrementing counter generated at message creation time in `Agent.js`). Use that `id` as the key on both the outer `div` and inner `Box`. Remove the duplicate `key` prop — keys only need to appear on the outermost element in a map.

---

#### RSPA-EFFECT-002 — Notable: useEffect dependency array missing `session`

`Agent.js` line 56-58: the effect reads `session` but does not include it in the dependency array. While the `if (session === '')` guard limits execution to the initial empty state, ESLint `exhaustive-deps` will warn, and the pattern is fragile: if `session` is ever reset to `''` deliberately (e.g., session expiry), the effect will not re-fire because the dependency was not declared.

**Recommendation:** Add `session` to the dependency array: `}, [session])`. The guard condition ensures the session is only fetched when empty.

---

#### RSPA-STATE-001 — Notable: Dead state in Main class component

`Main.js` line 13: `this.state = { question: '', searchResults: [] }`. These properties are never read in `render()`, never updated, and no method references them. They appear to be scaffolding that was never cleaned up.

**Recommendation:** Remove the unused state properties. If search functionality is planned, add it when implemented.

---

#### RSPA-COMP-001 — Notable: Props object not destructured in ChatLayout

`ChatLayout.js` line 6: `export default function ChatLayout(messages)` — the parameter name is `messages`, which is the entire props object. Accessing `messages.messages` is the double-dereference that results. If the component is called with `<ChatLayout items={...} />` instead of `<ChatLayout messages={...} />` the access silently returns `undefined`.

**Recommendation:** Destructure the prop: `function ChatLayout({ messages })`. This makes the prop contract explicit and eliminates the confusing double-access.

---

#### RSPA-EFFECT-003 — Notable: No error handling on fetch calls

`Agent.js` lines 29-44 and 48-52: both fetch chains use `.then().then()` with no `.catch()`. If the network request fails, or if the response body is not valid JSON (causing `response.json()` to throw), the error is silently swallowed. The user sees a blank response or a stalled chat, with no feedback.

**Recommendation:** Add `.catch(error => { /* set an error state or display a user-facing message */ })` to both fetch chains. Introduce an `error` state variable and conditionally render an error banner.

---

#### RSPA-COMP-002 — Minor: Class component with misleading exported name

`Main.js` defines `class App extends Component` but the module is imported as `Main`. The class name `App` is shared with the functional component in `App.js`. While no runtime collision occurs, the inconsistency causes confusion for developers and static analysis tools.

**Recommendation:** Rename the class to `Main` to match the module file name and import alias.

---

#### RSPA-COMP-003 — Minor: console.log left in production code

`Agent.js` line 38: `console.log(res)` logs the full API response — including any sensitive support-chat content — to the browser console in production builds. CRA does not strip `console.log` by default.

**Recommendation:** Remove the `console.log` call. If response logging is needed for debugging, gate it behind `process.env.NODE_ENV === 'development'` or use a structured logger.

---

#### RSPA-TS-001 — Minor: No TypeScript

The project ships plain JavaScript with no TypeScript configuration or PropTypes. All component props, API response shapes, and message object structures are completely untyped.

**Recommendation:** Migrate to TypeScript or, as a lighter alternative, add PropTypes declarations for all components. TypeScript would catch the `ChatLayout` props access bug (RSPA-COMP-001) and the `res.resp` / `res.session_id` shape assumptions at compile time.

---

#### RSPA-MEMO-001 — Info: ChatLayout memoization opportunity

`ChatLayout` is a pure rendering component. Every keystroke in the `TextField` updates `chatPrompt` state in `SupportAgent`, which re-renders `ChatLayout` even though the `messages` prop has not changed. At demo scale this is imperceptible, but at scale with long conversation histories it would cause unnecessary list re-renders.

**Observation:** Wrapping `ChatLayout` in `React.memo` would prevent re-renders when `messages` has not changed. This is a future optimization opportunity, not an urgent fix.

---

#### RSPA-STATE-002 — Info: Redundant `defaultValue` on controlled TextField

`Agent.js` line 74: `defaultValue="How do I fix Access Denied with Azure Blob Storage?"` on the MUI `TextField` is redundant because the component is controlled via the `value={chatPrompt}` prop. React ignores `defaultValue` on controlled inputs; only `value` drives the displayed content.

**Observation:** Remove `defaultValue` to avoid confusion. The initial value is already set by the `useState` initializer on line 11.

---

## 9. Recommended Improvements

| Finding | Recommended Action | Priority |
|---|---|---|
| RSPA-EFFECT-001 | Add `AbortController` cleanup to `handleSession` useEffect and track cancellation in `handlePrompt` | High |
| RSPA-KEY-001 | Assign stable `id` to each message object at creation; use `id` as list key; remove duplicate key props | High |
| RSPA-EFFECT-003 | Add `.catch()` error handlers to both fetch calls; add `error` state for user-facing error display | High |
| RSPA-EFFECT-002 | Add `session` to the useEffect dependency array | Medium |
| RSPA-COMP-001 | Destructure props in `ChatLayout`: `function ChatLayout({ messages })` | Medium |
| RSPA-STATE-001 | Remove unused `question` and `searchResults` from `Main` class state | Medium |
| RSPA-COMP-002 | Rename class `App` in `Main.js` to `Main` | Low |
| RSPA-COMP-003 | Remove `console.log(res)` from production code path | Low |
| RSPA-TS-001 | Add TypeScript (or PropTypes) to enforce component contracts | Low |
| RSPA-MEMO-001 | Wrap `ChatLayout` in `React.memo` when conversation history grows | Deferred |
| RSPA-STATE-002 | Remove `defaultValue` from controlled TextField | Deferred |

---

## Footer

This review is based on static analysis of source files only. No build was executed and no runtime behavior was observed. Findings reflect code patterns identified at the time of the review run (2026-03-21) and may not reflect runtime state or environment-specific behavior.

**Steward:** React SP Practices Steward (`react-sp-practises-steward`) — PREFIX `RSPA`