[Graphite MQ] Draft PR GROUP:spec_7c321a (PRs 3259, 3229, 3260)

[graphite-app]

This draft PR was created by the Graphite merge queue.
Trunk will be fast forwarded to the HEAD of this PR when CI passes, and the original PRs will be closed.

The following PRs are included in this draft PR:

• #3259
• #3229
• #3260

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-inspector	Ready	Preview	Comment	Oct 25, 2025 9:27am

2 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
rivet-cloud	Ignored			Oct 25, 2025 9:27am
rivet-site	Ignored	Preview		Oct 25, 2025 9:27am

[claude]

PR Review Summary

This is a Graphite merge queue PR combining three PRs (#3259, #3229, #3260) focused on bringing back TypeScript type checking and fixing various type errors in the frontend.

✅ Positive Changes

TypeScript Type Safety Improvements:

• Added TypeScript type checking to CI pipeline (.github/workflows/fe-pull-request.yml:19-28)
• Updated to React 19 type definitions for improved type safety (frontend/package.json:89-90)
• Removed unused imports across multiple files, improving code cleanliness
• Added proper type casting with QueryKey and UseQueryOptions types throughout data providers

Bug Fixes:

• Fixed invalid toggle behavior in connect-manual-frame.tsx:36-38 where the logic was inverted
• Fixed headers field array schema from z.array(z.string()) to z.array(z.array(z.string())) in edit-runner-config-form.tsx:29
• Added proper null checks and type guards (e.g., hasMetadataProvider function)
• Fixed React key props to use indexes instead of potentially non-unique values

Code Quality:

• Removed unused @ts-expect-error suppressions where possible
• Improved type definitions across data providers
• Better handling of optional/nullable values

⚠️ Issues & Concerns

1. Critical Bug - Toggle Logic is Broken
frontend/src/app/dialogs/connect-manual-frame.tsx:36-38

onValueChange={(mode) => {
    setMode(mode === "serverfull" ? "serverless" : "serverfull");
}}

This logic is backwards! When the user clicks "serverless", the function receives "serverless" as the mode, then sets it to "serverfull". This will make the toggle not work correctly. It should be:

onValueChange={(mode) => {
    if (mode) setMode(mode);
}}

Or simply:

onValueChange={setMode}

2. Type Safety Concerns with @ts-expect-error
frontend/src/app/data-providers/cloud-data-provider.tsx:22-23,30-31

// @ts-expect-error
fetcher: async (args) => {

While I understand this may be due to library type incompatibilities, adding @ts-expect-error without a descriptive comment explaining WHY defeats the purpose of enabling TypeScript checking. Consider adding comments like:

// @ts-expect-error - Fern SDK types are incompatible with our custom fetcher signature

3. Unused Import
frontend/src/app/data-providers/engine-data-provider.tsx:26

import z from "zod";

This import appears only to be used for the hasMetadataProvider function. Should verify this is necessary or use a more specific import.

4. Form Field Value Synchronization
frontend/src/app/forms/edit-runner-config-form.tsx:229,261
Using watch() in the render for controlled inputs is inefficient. The form library should handle this automatically via the field registration. Consider using the field's value directly from useFieldArray:

// Current (inefficient):
value={watch(`headers.${index}.0`)}

// Better:
{...register(`headers.${index}.0`)}

5. Missing Error Handling
Several places now have type assertions but don't handle the error cases:

• frontend/src/app/data-providers/engine-data-provider.tsx:661 - Returns empty string on missing token without logging
• Type guards like hasMetadataProvider silently return false on invalid data

6. Potential Runtime Issues
frontend/src/app/runner-config-table.tsx:146

<DiscreteCopyButton value={config.serverless?.url || ""}>

Copying an empty string when URL is undefined might confuse users. Consider disabling the button instead.

📋 Recommendations

1. Fix the toggle bug immediately - This will break user interaction
2. Add descriptive comments to all @ts-expect-error suppressions explaining the reason
3. Consider adding runtime validation where type assertions are made
4. Review form field implementations for performance (use register instead of manual watch + onChange)
5. Add error logging where tokens or credentials might be missing
6. Test the inspector integration thoroughly as the token handling has changed significantly

🔒 Security

• Token handling changes look reasonable
• Proper cleanup in useEffect hooks with abort controllers
• No obvious security issues introduced

🧪 Testing

The PR adds TypeScript checking to CI, which is excellent. However:

• Consider adding integration tests for the toggle behavior
• Test the form header inputs with various data types
• Verify inspector token flow works with both cloud and engine modes

Overall Assessment

The PR makes good progress on type safety, but has a critical bug in the toggle logic that needs to be fixed before merging. The TypeScript improvements are valuable, though the liberal use of @ts-expect-error without explanation is concerning. The code quality improvements (removing unused imports, better types) are positive.

Recommendation: Request changes - Fix the toggle bug and add comments to type suppressions before merging.