Skip to content

refactor(agent-streaming): Mount React app#119

Merged
agduan merged 1 commit into
agent-streaming/4-react-componentsfrom
agent-streaming/5-react-app-mount
Jun 16, 2026
Merged

refactor(agent-streaming): Mount React app#119
agduan merged 1 commit into
agent-streaming/4-react-componentsfrom
agent-streaming/5-react-app-mount

Conversation

@agduan

@agduan agduan commented Jun 12, 2026

Copy link
Copy Markdown
Contributor
  • refactor: Add React entry point
  • refactor: Update index.html with React root
  • chore: Delete unused script.ts

@agduan agduan requested a review from nohe427 June 12, 2026 03:54

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request migrates the client-side chat interface from static HTML to a React-based application, introducing a main App component to manage chat state and stream responses from the backend. The review feedback highlights critical improvements for the streaming implementation, including adding an AbortController to prevent memory leaks on unmounted components, allowing request cancellation, and changing the scroll behavior from 'smooth' to 'auto' to avoid performance degradation during rapid message updates.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread agent-streaming/src/client/App.tsx
@agduan agduan force-pushed the agent-streaming/5-react-app-mount branch from 7db4252 to 612bacd Compare June 12, 2026 21:09
@agduan agduan force-pushed the agent-streaming/4-react-components branch from 5a0537b to 542b08b Compare June 12, 2026 21:09
@agduan agduan force-pushed the agent-streaming/5-react-app-mount branch from 612bacd to 3fa3234 Compare June 12, 2026 21:30
@agduan agduan force-pushed the agent-streaming/4-react-components branch from 542b08b to 2c46183 Compare June 12, 2026 21:30
@HYACCCINT

Copy link
Copy Markdown

Very clean, looks good to me, one thing that I might recommend is removing the script files since they're no longer being called. LGTM.

@agduan agduan force-pushed the agent-streaming/5-react-app-mount branch from 3fa3234 to afc01b3 Compare June 13, 2026 05:24
@agduan agduan force-pushed the agent-streaming/4-react-components branch from 2c46183 to 673ceeb Compare June 13, 2026 05:24
@agduan agduan force-pushed the agent-streaming/5-react-app-mount branch from afc01b3 to ced3cbd Compare June 13, 2026 05:35
@agduan agduan force-pushed the agent-streaming/4-react-components branch from 673ceeb to 77c4cc3 Compare June 15, 2026 04:47
@agduan agduan force-pushed the agent-streaming/5-react-app-mount branch from ced3cbd to 7c8b888 Compare June 15, 2026 04:47

@jhuleatt jhuleatt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice! I left some ideas to help simplify this

Comment thread agent-streaming/src/client/App.tsx Outdated
Comment thread agent-streaming/src/client/App.tsx Outdated
Comment thread agent-streaming/src/client/App.tsx Outdated
Comment thread agent-streaming/src/client/App.tsx Outdated
@agduan agduan force-pushed the agent-streaming/5-react-app-mount branch from 7c8b888 to c91b8ad Compare June 15, 2026 18:19
@agduan agduan force-pushed the agent-streaming/4-react-components branch from 77c4cc3 to 5d39572 Compare June 15, 2026 18:19
Comment thread agent-streaming/src/client/App.tsx Outdated
const chunk = JSON.parse(jsonStr) as StreamChunk;
handleStreamChunk(chunk);
} catch (err) {
console.error('Failed to parse SSE JSON:', err);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Will swallowing this error lead to weirdly formatted messages (a message missing a data chunk in the middle)? Should this throw instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, good idea! I resolved by removing the inner try/catch block entirely so parsing errors throw immediately & trigger the outer catch block.

Comment thread agent-streaming/src/client/App.tsx Outdated
Comment on lines +75 to +76
if (trimmed.startsWith('data: ')) {
const jsonStr = trimmed.slice(6);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion to make this less brittle:

Suggested change
if (trimmed.startsWith('data: ')) {
const jsonStr = trimmed.slice(6);
const dataPrefix = 'data: ';
if (trimmed.startsWith(dataPrefix)) {
const jsonStr = trimmed.slice(dataPrefix.length);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For my own learning, is this data: prefix a Genkit thing? Or another standard?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done! data is one of the standard field names in HTML5's SSE protocol (which enforces the field: value format for events).

Comment thread agent-streaming/src/client/App.tsx Outdated
Comment on lines +64 to +65
const { value, done } = await reader.read();
if (done) break;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

do you need to check controller.signal.aborted here before the next read() (and break if aborted is true), or does reader.read() do that automatically?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done automatically!

Comment thread agent-streaming/src/client/App.tsx Outdated
buffer += decoder.decode(value, { stream: true });
const lines = buffer.split('\n');

// Keep the last partial line in the buffer

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you add a "why" to the end of this comment? If I'm understanding this, maybe:

Suggested change
// Keep the last partial line in the buffer
// Keep only the last partial line in the buffer to concat with the next chunk

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done with // Store the last partial line in the buffer to prevent parsing errors if a chunk is split mid-line

Comment thread agent-streaming/src/client/App.tsx
Comment thread agent-streaming/src/client/App.tsx Outdated

for (const line of lines) {
const trimmed = line.trim();
if (trimmed.startsWith('data: ')) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What should happen if a line doesn't start with data: ? It looks like it is just ignored at the moment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The server is streaming response chunks using the SSE format (data: <JSON stuff>\n\n). So when the buffer is split on newlines, we either get valid data starting with data: that we should parse, or empty lines that we can just ignore

I think edge cases should already be handled in the rest of the program - invalid JSON gets caught and server errors are still prefixed with data

Comment thread agent-streaming/src/client/App.tsx Outdated
if (trimmed.startsWith('data: ')) {
const jsonStr = trimmed.slice(6);
try {
const chunk = JSON.parse(jsonStr) as StreamChunk;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you could do some checking to make sure it actually is a StreamChunk (or that could be overkill for a sample)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Probably going to go with overkill since we already have validation on server side, but def will consider as best practice for future apps

- refactor: Add React entry point
- refactor: Update index.html with React root
- chore: Delete unused script.ts
@agduan agduan force-pushed the agent-streaming/5-react-app-mount branch from c91b8ad to 9c059ae Compare June 16, 2026 17:13
@agduan agduan force-pushed the agent-streaming/4-react-components branch from 5d39572 to 35721ec Compare June 16, 2026 17:13
@agduan agduan merged commit 32b04ee into agent-streaming/4-react-components Jun 16, 2026
4 of 7 checks passed
@agduan agduan deleted the agent-streaming/5-react-app-mount branch June 16, 2026 18:10
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