Skip to content

Conversation

@jog1t
Copy link
Contributor

@jog1t jog1t commented Oct 24, 2025

No description provided.

@vercel
Copy link

vercel bot commented Oct 24, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivet-inspector Ready Ready Preview Comment Oct 24, 2025 11:01pm
2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Oct 24, 2025 11:01pm
rivet-site Ignored Ignored Preview Oct 24, 2025 11:01pm

Copy link
Contributor Author

jog1t commented Oct 24, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@graphite-app
Copy link
Contributor

graphite-app bot commented Oct 24, 2025

Graphite Automations

"Test" took an action on this PR • (10/24/25)

1 assignee was added to this PR based on Kacper Wojciechowski's automation.

@jog1t jog1t force-pushed the 10-25-fix_fe_invalid_types branch from 596b927 to d313e2a Compare October 24, 2025 22:41
@claude
Copy link

claude bot commented Oct 24, 2025

PR Review: fix(fe): invalid types

Summary

This PR fixes TypeScript type errors across the frontend codebase, focusing on data providers, UI components, and API integration. Overall, the changes improve type safety, though there are a few areas that could be refined.

Code Quality & Best Practices

Strengths:

  • Systematic fix of type errors across the codebase
  • Improved workflow CI configuration for type checking
  • Added proper utility functions for type guards

Areas for Improvement:

  1. Avoid @ts-expect-error suppressions (cloud-data-provider.tsx:22-31)

    // @ts-expect-error
    fetcher: async (args) => {
        // ...
        return await fetcher(
            // @ts-expect-error
            args
        );
    },

    Issue: Using @ts-expect-error suppresses type checking without addressing the root cause. This indicates a type mismatch between the fetcher interface and usage.
    Recommendation: Define proper types for the fetcher args or use a type assertion with explanation of why it's safe.

  2. Inconsistent optional chaining (cloud-data-provider.tsx:24-27)

    Object.keys(args.headers || {}).forEach((key) => {
        if (key.toLowerCase().startsWith("x-fern-")) {
            delete args.headers?.[key];
        }
    });

    Issue: Using || {} for the iteration but ?. for deletion is inconsistent. If headers can be undefined, both should handle it.

  3. React key should be stable (plan-card.tsx:54)

    {features?.map((feature, index) => (
        <li key={index}>

    Issue: While using index as key is acceptable here since the list appears static, the original feature.label was more semantic.
    Recommendation: If features can have duplicate labels, consider key={${feature.icon}-${index}} for better semantics.

  4. Weak type guard (connect-manual-frame.tsx:36)

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

    Issue: This toggle logic is fragile - if the value is neither "serverfull" nor "serverless", it will set to "serverfull".
    Recommendation: Add type guard or assertion to ensure mode is valid before toggling.

  5. Redundant undefined checks (engine-data-provider.tsx:709-725)
    The new hasProvider function has good type safety with the hasMetadataProvider guard, but could be more concise:

    return configs.some(([, config]) =>
        Object.values(config.datacenters).some(
            (datacenter) =>
                datacenter.metadata && 
                hasMetadataProvider(datacenter.metadata) && 
                datacenter.metadata.provider &&  // This check is redundant after hasMetadataProvider
                providers.includes(datacenter.metadata.provider),
        ),
    );

Potential Bugs

  1. Field array type safety (edit-runner-config-form.tsx:182-189)

    const { control, setValue, watch } = useFormContext();
    const { fields, append, remove } = useFieldArray({
        name: "headers",
        control,
    });

    Issue: Using useFormContext() without generic type parameter loses type safety. The watch and setValue calls later won't be type-checked.
    Recommendation: Pass the form type to useFormContext<FormValues>().

  2. Missing import cleanup (engine-data-provider.tsx:1-26)
    The PR removed unused imports but added new ones. The FetchFunction, QueryOptions imports were removed, but line 26 adds import z from "zod" - ensure zod is in package.json dependencies.

  3. Unsafe type coercion (engine-data-provider.tsx:661)

    return (ls.engineCredentials.get(getConfig().apiUrl) || "") as string;

    Issue: The as string is redundant since the fallback is already "".

  4. ResizeObserver ref type issue (actor-events.tsx:177)

    // @ts-expect-error -- TS2322 -- Type 'HTMLDivElement' is not assignable to type 'Element | null'.
    ref,

    Issue: This suggests a genuine type incompatibility. The ResizeObserver API expects Element, but the ref is HTMLDivElement | null.
    Recommendation: Either fix the ref type or use a proper type assertion explaining why HTMLDivElement is safe.

Performance Considerations

  1. Query option functions recreated on every render
    The data provider functions like engineAdminTokenQueryOptions() are recreated on every call. This is fine for TanStack Query's structural sharing, but be aware of potential reference equality issues in dependencies.

  2. Unnecessary re-renders from watch (edit-runner-config-form.tsx:229, 261)

    value={watch(`headers.${index}.0`)}

    Each field watches the form, which could cause re-renders. Consider using useController for better performance with field arrays.

Security Concerns

  1. Header manipulation (cloud-data-provider.tsx:24-27, engine-data-provider.tsx:50-54)
    Removing x-fern- headers appears intentional, but ensure this doesn't bypass security controls. Document why these headers are being stripped.

  2. Token handling looks appropriate
    Token caching with appropriate stale/gc times (5-15 minutes) is good. Secret tokens use proper query isolation.

Test Coverage

Missing:

  • No test changes included for the new utility functions (hasMetadataProvider, hasProvider, getRegionLabel)
  • Type guard functions should have unit tests to verify edge cases

Recommendation: Add tests for:

describe('hasMetadataProvider', () => {
  it('should return true for valid metadata with provider', () => {
    expect(hasMetadataProvider({ provider: 'aws' })).toBe(true);
  });
  
  it('should return false for invalid metadata', () => {
    expect(hasMetadataProvider({ foo: 'bar' })).toBe(false);
  });
});

CI/CD Improvements

The workflow changes are good:

  • Using pnpm biome instead of assuming biome is installed
  • Building engine before type checking (correct dependency order)
  • Consistent pnpm usage

Minor Issues

  1. Missing trailing newline in .github/workflows/fe-pull-request.yml
  2. Unused parent spread in createNamespaceContext (engine-data-provider.tsx:134)

Summary Recommendations

Must Fix:

  1. Remove @ts-expect-error suppressions and fix underlying type issues
  2. Add type parameter to useFormContext<FormValues>() in edit-runner-config-form.tsx
  3. Fix ResizeObserver ref type properly instead of suppressing error

Should Fix:

  1. Make toggle logic more robust in connect-manual-frame.tsx
  2. Add tests for new utility functions
  3. Document why x-fern headers are being removed

Nice to Have:

  1. Optimize field array performance with useController
  2. Use more semantic keys instead of indices where possible
  3. Remove redundant type assertions

Overall this is a solid type safety improvement, but addressing the @ts-expect-error suppressions and adding proper types would make it production-ready.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 24, 2025

More templates

@rivetkit/actor

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/actor@3260

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3260

@rivetkit/core

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/core@3260

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3260

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3260

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3260

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3260

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3260

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3260

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3260

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3260

commit: dc71675

@claude
Copy link

claude bot commented Oct 24, 2025

Pull Request Review: fix(fe): invalid types

Summary

This PR addresses TypeScript type errors across the frontend codebase including CI configuration fixes, type annotations, data provider improvements, and component updates.


Positive Aspects

  1. CI Improvements: Simplified GitHub Actions workflow
  2. Type Safety: Added type guards (hasMetadataProvider, hasProvider)
  3. Better Null Handling: Improved undefined value handling
  4. Code Organization: Moved vite environment declarations properly

Critical Issues

  1. LOGIC BUG (connect-manual-frame.tsx:36-38)

    • Mode toggle logic is inverted - clicking serverfull sets serverless
    • Should be: setMode(mode as serverless | serverfull)
  2. Type Safety Suppressions (cloud-data-provider.tsx:21-30)

    • Multiple @ts-expect-error without explanations
    • Need comments explaining why types are incompatible
  3. Unsafe Array Access (edit-runner-config-form.tsx:229, 261)

    • Using watch() instead of field value directly

Moderate Issues

  1. API Method Naming (engine-data-provider.tsx:406+)

    • Changed from client.runnerConfigs.X to client.runnerConfigsX
    • Verify this matches actual API contract
  2. React Key Anti-pattern (plan-card.tsx:54)

    • Using array index instead of feature.label as key
  3. Unused Parameter (engine-data-provider.tsx:131)

    • parent parameter spread but never used

Security Concerns

  • Header stripping without documentation (cloud-data-provider.tsx:23-27)

Missing

  • Tests for new utility functions
  • PR description is empty
  • Error handling in callAction function

Overall Assessment

Recommendation: Request Changes

Before merging:

  1. Fix inverted mode toggle logic (critical bug)
  2. Document all @ts-expect-error suppressions
  3. Verify API method naming changes
  4. Add tests for utility functions
  5. Fill out PR description

@NathanFlurry NathanFlurry changed the base branch from 10-20-fix_make_inspector_working_again to graphite-base/3260 October 25, 2025 09:19
@graphite-app
Copy link
Contributor

graphite-app bot commented Oct 25, 2025

Merge activity

  • Oct 25, 9:24 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Oct 25, 9:25 AM UTC: CI is running for this pull request on a draft pull request (#3262) due to your merge queue CI optimization settings.
  • Oct 25, 9:26 AM UTC: Merged by the Graphite merge queue via draft PR: #3262.

graphite-app bot pushed a commit that referenced this pull request Oct 25, 2025
@graphite-app graphite-app bot closed this Oct 25, 2025
@graphite-app graphite-app bot deleted the 10-25-fix_fe_invalid_types branch October 25, 2025 09:26
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.

2 participants