Skip to content

7. Add playground input controls and validation#3111

Open
anish-sahoo wants to merge 3 commits into
cog-playgroundfrom
playground/4b-inputs
Open

7. Add playground input controls and validation#3111
anish-sahoo wants to merge 3 commits into
cog-playgroundfrom
playground/4b-inputs

Conversation

@anish-sahoo

@anish-sahoo anish-sahoo commented Jul 14, 2026

Copy link
Copy Markdown
Member

@anish-sahoo anish-sahoo requested a review from a team as a code owner July 14, 2026 18:26
@ask-bonk

ask-bonk Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

I'm Bonk, and I've done a quick review of your PR.

This PR adds schema-driven input controls, validation utilities, and tests for the Cog playground. The code is well-structured and the test coverage is comprehensive.

I found a few actionable issues:

  1. useEffectEvent is an experimental React API — This hook is not available in stable React releases and was excluded from React 19. Using it will break builds or runtime.

  2. Busy state leak on unmount — FileInputControl doesn't clear busy state when unmounting while a file read is in-flight.

  3. Render-phase ref mutation — usePlaygroundInput assigns to a ref during render, which violates React's purity rules and can cause issues with concurrent features.


FileInputControl.tsx

Replace the experimental useEffectEvent with the standard ref-synchronization pattern and clear busy state on unmount:

import { useEffect, useRef, useState } from "react";

// ... inside FileInputControl component ...

  const onBusyChangeRef = useRef(props.onBusyChange);
  useEffect(() => {
    onBusyChangeRef.current = props.onBusyChange;
  });

  useEffect(() => {
    if (!disabled) return;
    readToken.current += 1;
    onBusyChangeRef.current(false);
  }, [disabled]);
  useEffect(
    () => () => {
      readToken.current += 1;
      onBusyChangeRef.current(false);
    },
    [],
  );

usePlaygroundInput.ts

Move the render-phase ref mutation into a useEffect:

  const connectionState = useRef({ target, document, capabilities });
  useEffect(() => {
    connectionState.current = { target, document, capabilities };
  }, [target, document, capabilities]);

@ask-bonk

ask-bonk Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

I'm Bonk, and I've done a quick review of your PR.

This PR adds schema-driven input controls, validation utilities, and tests for the Cog playground. The code is well-structured and the test coverage is comprehensive.

I found a few actionable issues:

  1. useEffectEvent is an experimental React API — This hook is not available in stable React releases and was excluded from React 19. Using it will break builds or runtime.
  2. Busy state leak on unmountFileInputControl doesn't clear busy state when unmounting while a file read is in-flight.
  3. Render-phase ref mutationusePlaygroundInput assigns to a ref during render, which violates React's purity rules and can cause issues with concurrent features.

I've posted the review with concrete code suggestions to PR #3111.

github run

@anish-sahoo

Copy link
Copy Markdown
Member Author

Review disposition:

  • useEffectEvent is stable in the pinned React ^19.2.4 (resolved React 19.2.7), so it remains.
  • Fixed the Form busy-state leak on unmount in 6a86c55, with regression coverage.
  • Moved the connection-state ref update to useLayoutEffect in 6a86c55 to avoid render-phase mutation.

These are top-level comments, so GitHub provides no review thread to resolve.

@ask-bonk ask-bonk 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.

I'm Bonk, and I've done a quick review of your PR.

This PR adds schema-driven input controls, validation utilities, and tests for the Cog playground. The code is well-structured and the test coverage is comprehensive.

I found a few actionable issues:

  1. useEffectEvent is an experimental React API — This hook is not available in stable React releases and was excluded from React 19. Using it will break builds or runtime.
  2. Busy state leak on unmount — FileInputControl doesn't clear busy state when unmounting while a file read is in-flight.
  3. Render-phase ref mutation — usePlaygroundInput assigns to a ref during render, which violates React's purity rules and can cause issues with concurrent features.

Comment thread playground/src/features/inputs/components/FileInputControl.tsx
Comment thread playground/src/features/inputs/components/FileInputControl.tsx
Comment thread playground/src/features/inputs/components/FileInputControl.tsx
Comment thread playground/src/features/inputs/components/FileInputControl.tsx
Comment thread playground/src/features/inputs/hooks/usePlaygroundInput.ts
@ask-bonk

ask-bonk Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

I've posted a review to PR #3111 with three actionable issues:

  1. useEffectEvent is experimentalFileInputControl.tsx imports this unstable React hook. It was excluded from React 19 stable and will break builds.
  2. Busy state leak on unmountFileInputControl increments the read token on unmount but never calls onBusyChange(false), so the parent stays busy forever.
  3. Render-phase ref mutationusePlaygroundInput syncs a ref in useLayoutEffect; moving it to useEffect is safer for concurrent React.

All three have concrete code suggestions attached to the review.

github run

@anish-sahoo

Copy link
Copy Markdown
Member Author

Follow-up disposition: useEffectEvent is stable in the pinned React 19.2 version; the aggregate busy state is cleared by the existing InputForm unmount cleanup with regression coverage; and useLayoutEffect intentionally keeps the committed connection freshness guard current before a stale async validation can settle. All five attached review threads are resolved.

@ask-bonk

ask-bonk Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

LGTM

github run

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.

1 participant