fix(fe): bring back ts type check

[jog1t]

No description provided.

[vercel]

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

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

2 Skipped Deployments

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

[jog1t]

• fix(fe): invalid types #3260
• fix: make inspector working again #3229
• fix(fe): bring back ts type check #3259 👈 (View in Graphite)
• ci: fix release scripts for engine #3258
• chore(scripts): add pg backup & restore scripts for dev #3251
• feat(examples): cursors with raw websockets example #3245
• feat(examples-cursors): cursors example #3244
• feat(core): allow routing traffic to actors via path #3250
• fix: fix biome ignore path #3249
• chore: fix rivetkit release script #3242 : 1 other dependent PR (#3243 )
• main

---

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]

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.

[claude]

PR Review - TypeScript Type Checking Re-enablement

Overview

This PR re-enables TypeScript type checking in the frontend CI pipeline and fixes several issues to make the codebase pass type checks. Overall, this is a positive change that improves code quality and type safety.

Positive Changes

CI/CD Improvements - Added dedicated tsc job to run TypeScript type checking in CI. Fixed path patterns to match current monorepo structure. Added working-directory to Biome check.

Dependency Fix - Changed rivetkit dependency from "" to "workspace:" which is the correct pnpm workspace syntax.

Code Quality - Removed unused imports (Link, Button, Card components, RootLayout) improving build performance and reducing bundle size.

Query Key Consistency - Simplified query keys by passing entire opts object instead of destructuring, which is better for type safety and handles undefined cases properly.

Areas of Concern

TypeScript Error Suppression - Lines 135-138 in actor-driver.ts use @ts-expect-error without explanatory comments. This bypasses type safety and creates technical debt. Recommend adding comments explaining WHY these errors exist, or better yet, fix the underlying type mismatch.

Missing Type Investigation - The PR doesn't show what the actual type errors are. Without seeing the signature mismatch, it's hard to assess whether @ts-expect-error is masking a real bug.

Summary

Overall Assessment: Approve with minor suggestions

The PR successfully re-enables TypeScript checking and fixes most issues cleanly. The only concern is the use of @ts-expect-error without proper documentation.

Recommended before merge:

1. Add explanatory comments to the @ts-expect-error suppressions
2. Add trailing newline to workflow file
3. Consider creating a follow-up issue to properly type the fetch and websocket methods

Recommended after merge:

1. Investigate and fix the root cause of the type errors in actor-driver.ts
2. Consider adding similar type-check job for other TypeScript packages in the monorepo

Great work on improving type safety!

[claude]

Pull Request Review: Bring back TypeScript type checking

Thanks for working on bringing back TypeScript type checking to the frontend! This is an important improvement for code quality. Here's my detailed review:

✅ Positive Changes

1. GitHub Actions CI Enhancement (.github/workflows/fe-pull-request.yml)

   • Good addition of TypeScript compiler check in CI
   • Proper path coverage including engine/sdks/typescript/** and rivetkit-typescript/**
   • Clean separation of quality and tsc jobs

2. React Type Upgrades (frontend/package.json)

   • Upgrading to React 19 types is forward-looking
   • Using workspace:* for rivetkit is the correct approach for monorepo dependencies

3. Type Safety Improvements

   • Adding explicit type annotations throughout (e.g., QueryKey, UseQueryOptions, etc.)
   • Fixing unsafe type assertions and adding proper type guards

🐛 Critical Issues

1. Missing Error Handling in useAsyncMemo Hook

Location: frontend/src/components/hooks/use-async-memo.tsx:11-15

promise.then((val) => {
  if (!cancel) {
    setVal(val)
  }
})

Issue: The promise chain has no error handling. If the async factory throws an error, it will be an unhandled promise rejection.

Recommendation:

promise.then((val) => {
  if (!cancel) {
    setVal(val)
  }
}).catch((error) => {
  console.error('useAsyncMemo error:', error)
  // Consider: setVal(initial) to reset to initial value on error
})

2. Potential Race Condition in useAsyncMemo

Location: frontend/src/components/hooks/use-async-memo.tsx:7-19

Issue: If dependencies change rapidly, multiple promises may be in flight, and the cancellation flag only prevents setting state, but doesn't actually cancel the promise execution. The last resolved promise (not necessarily from the latest deps) could win.

Recommendation: Consider using AbortController for actual promise cancellation if the factory supports it, or add a sequence counter to ensure only the latest promise updates state.

3. Type Safety Regression with as any

Location: frontend/src/app/data-providers/engine-data-provider.tsx:72

queryKey: ["namespaces"] as any,

Issue: Using as any defeats the purpose of TypeScript type checking. This should be as QueryKey or properly typed.

Same issue in: Multiple locations throughout the codebase where as QueryKey is used but could be better typed.

⚠️ Moderate Issues

4. Unsafe Null Coalescing in Filters

Location: frontend/src/components/actors/actors-list.tsx:231,333,336,349

...remove(prev || {})

Issue: If prev is null or undefined, falling back to {} might not be type-safe depending on what remove expects. The same pattern appears in multiple places.

Recommendation: Ensure remove function properly handles empty objects, or provide a properly typed default value.

5. Removed Parameter Without Context

Location: frontend/src/app/data-providers/engine-data-provider.tsx:134

// Before:
{ namespace: string; namespaceId: string }

// After:
{ namespace: string; }

Issue: The namespaceId parameter was removed from createNamespaceContext. Ensure all callers have been updated and this ID is not needed elsewhere.

6. Type Assertion in JSON Component

Location: frontend/src/components/json/index.tsx:667

const Comp = as as 'input';

Issue: This is a confusing type assertion. The parameter as can be "input" | "textarea", but it's being forced to 'input'. This will break if as="textarea" is passed.

Recommendation: Either support both types properly or handle them separately:

if (as === 'textarea') {
  return <textarea ref={ref as any} ... />
}
return <input ref={ref} ... />

💡 Suggestions for Improvement

7. Formatting and Style

Location: frontend/src/components/hooks/use-async-memo.tsx

• Missing spaces around braces and after commas (doesn't follow project style)
• Should be: import { DependencyList, useEffect, useState } from 'react'
• Add missing cleanup return type comment

8. TypeScript Strictness

Consider enabling stricter TypeScript compiler options in tsconfig.json:

{
  "compilerOptions": {
    "strict": true,
    "noUncheckedIndexedAccess": true,
    "noImplicitAny": true
  }
}

9. Test Coverage

The new useAsyncMemo hook should have unit tests covering:

• Successful async resolution
• Error handling
• Cleanup on unmount
• Rapid dependency changes (race conditions)
• Initial value behavior

10. Missing Workflow Trailing Newline

Location: .github/workflows/fe-pull-request.yml:34

The file is missing a trailing newline (notice the \ No newline at end of file in the diff).

🔒 Security Considerations

No major security concerns identified. The type improvements actually enhance security by catching potential runtime errors at compile time.

📊 Performance Considerations

The useAsyncMemo hook is called in hot paths (e.g., streaming hooks in frontend/src/components/actors/queries/actor.ts). Consider:

• Memoizing the factory function more aggressively
• Adding metrics to track how often the async factory is called
• Potential memory leaks if promises are never resolved/rejected

Summary

This PR makes important progress toward type safety, but has some critical issues that should be addressed:

Must Fix Before Merge:

1. Add error handling to useAsyncMemo
2. Fix the as type assertion in JSON component
3. Replace as any with proper types

Should Fix:
4. Add tests for useAsyncMemo
5. Address race condition concerns
6. Fix code formatting
7. Add trailing newline to workflow file

Nice to Have:
8. Stricter TypeScript compiler options
9. Better documentation for the new hook

Overall, this is a valuable improvement to the codebase. With the critical issues addressed, this will significantly improve type safety and developer experience.

[claude]

PR Review: fix(fe): bring back ts type check

Summary

This PR successfully re-enables TypeScript type checking in the frontend CI pipeline and fixes numerous type errors throughout the codebase. The changes are well-structured and improve type safety across the application.

✅ Positive Changes

1. CI/CD Improvements

• ✅ Added TypeScript type checking job to the GitHub Actions workflow (.github/workflows/fe-pull-request.yml:21-35)
• ✅ Updated path triggers to include engine/sdks/typescript/** and rivetkit-typescript/**
• ✅ Proper working directory configuration

2. Type Safety Enhancements

• ✅ Updated React types to v19 (@types/react and @types/react-dom)
• ✅ Fixed numerous implicit any types by adding proper type annotations
• ✅ Added explicit QueryKey type assertions throughout data providers
• ✅ Improved generic type constraints in multiple components

3. Code Quality

• ✅ Removed unused imports (e.g., frontend/src/app.tsx, frontend/src/queries/utils.ts)
• ✅ Better nullability handling with optional chaining and null checks
• ✅ Proper workspace dependency usage: rivetkit: "workspace:*" in frontend/package.json:123

🔍 Issues & Concerns

1. useAsyncMemo Hook - Missing Error Handling ⚠️

File: frontend/src/components/hooks/use-async-memo.tsx:10-15

const promise = factory()
if (promise === undefined || promise === null) return
promise.then((val) => {
  if (!cancel) {
    setVal(val)
  }
})

Issues:

• No .catch() handler - promise rejections will be unhandled
• Could cause "Unhandled Promise Rejection" warnings in development
• May lead to silent failures in production

Recommendation:

promise.then((val) => {
  if (!cancel) {
    setVal(val)
  }
}).catch((error) => {
  console.error('useAsyncMemo error:', error)
  // Consider: call an error callback or set error state
})

2. Type Safety Concerns

a. Unsafe Type Assertions

File: frontend/src/components/actors/actor-queries-context.tsx:180

return await response.json() as {events: RecordedRealtimeEvent[]};

• Using as assertion bypasses type checking
• If API returns different structure, runtime errors may occur
• Consider using a type guard or validation library like zod

b. Generic Type Constraint Missing

File: frontend/src/components/json/index.tsx:665

const Comp = as as 'input';

• Double type assertion pattern is a code smell
• Suggests the component architecture might need refactoring

c. Loose any Types

File: frontend/src/app/data-providers/engine-data-provider.tsx:72

queryKey: ["namespaces"] as any,

• Using as any defeats the purpose of TypeScript
• Should use as QueryKey or proper type instead

3. Potential Race Condition ⚠️

File: frontend/src/components/actors/queries/actor.ts:94-100

useEffect(() => {
  const controller = new AbortController();
  if (!opts.enabled || !url) {
    controller.abort();
    return () => controller.abort();
  }
  // ... async operations

Issue: The url comes from useAsyncMemo which may not be immediately available. The stream might start before the URL is resolved, or change while a connection is active.

Recommendation: Consider adding a stable loading state or ensuring URL is defined before enabling the effect.

4. Redundant Null Check

File: frontend/src/components/actors/queries/actor.ts:102-105

async function establishConnection() {
  if(!url){
    return;
  }

This check is redundant since we already check !url on line 97.

5. Inconsistent mutationOptions Usage

• Some mutations use mutationOptions() helper: frontend/src/app/data-providers/engine-data-provider.tsx:362-384
• Others use object literals: frontend/src/app/data-providers/default-data-provider.tsx:252-267
• Recommendation: Use the mutationOptions() helper consistently for better type inference

🎯 Best Practices Alignment

✅ Follows Conventions

• Hard tabs preserved (per rustfmt.toml)
• Imports at top of files
• Structured logging patterns maintained
• Workspace dependencies properly configured

⚠️ Areas for Improvement

1. Error handling in async hooks
2. Reduce use of type assertions (as)
3. Consider runtime validation for API responses
4. Consistent mutation options patterns

🔒 Security

• ✅ No obvious security vulnerabilities introduced
• ✅ Token handling appears unchanged and secure
• ℹ️ Type assertions could potentially mask data validation issues

⚡ Performance

• ✅ No significant performance concerns
• ℹ️ useAsyncMemo creates a new promise on every dependency change - this is expected but worth noting
• ℹ️ Type-only changes have no runtime impact

🧪 Test Coverage

• ❌ No new tests added
• ℹ️ The PR title indicates this is a fix to bring back type checking
• Recommendation: Consider adding tests for the new useAsyncMemo hook

📝 Recommendations

High Priority

1. Add error handling to useAsyncMemo hook
2. Replace as any assertions with proper types
3. Add validation for API response types

Medium Priority

4. Make mutation options usage consistent
5. Remove redundant null checks
6. Add tests for useAsyncMemo

Low Priority

7. Consider refactoring the double type assertion in json/index.tsx
8. Document the async behavior of createActorInspector methods

✨ Overall Assessment

This is a solid PR that successfully addresses the TypeScript type checking issue. The changes improve type safety across the codebase, though there are opportunities to strengthen error handling and reduce reliance on type assertions.

Recommendation: ✅ Approve with minor suggested improvements

The issues identified are not blockers but should be addressed in follow-up work to maximize the benefits of strict type checking.

[claude]

PR Review: Bring Back TypeScript Type Checking

Overview

This PR successfully re-enables TypeScript type checking for the frontend and addresses numerous type safety issues. The changes are well-structured and improve code quality significantly.

✅ Strengths

1. CI/CD Improvements

   • Added TypeScript compiler checks to the GitHub Actions workflow (.github/workflows/fe-pull-request.yml:21-35)
   • Properly configured trigger paths to include relevant TypeScript packages
   • Good addition of the tsc job to catch type errors early

2. Type Safety Enhancements

   • Comprehensive addition of explicit QueryKey type assertions throughout data providers
   • Proper use of mutationOptions() wrapper for better type inference
   • Added explicit return type annotations (e.g., UseQueryOptions<string>) where needed
   • Upgraded to React 19 types (@types/react@^19, @types/react-dom@^19)

3. Smart Async Handling

   • Created useAsyncMemo hook (frontend/src/components/hooks/use-async-memo.tsx) to handle async factory functions in React hooks
   • Proper cleanup with cancellation tokens to prevent memory leaks and race conditions

4. Defensive Null Handling

   • Added null guards throughout (e.g., pick(state || {}), remove(prev || {}))
   • Better handling of undefined states in navigation functions

⚠️ Issues Found

1. Critical Bug: Undefined Variable Reference

Location: frontend/src/app/data-providers/engine-data-provider.tsx:193

queryKey: [
    { namespace, namespaceId },  // ❌ namespaceId is not defined
    ...def.regionQueryOptions(regionId).queryKey,
],

The variable namespaceId is referenced but was removed from the function signature (line 135). This should be:

queryKey: [
    { namespace },
    ...def.regionQueryOptions(regionId).queryKey,
],

2. Type Safety Concerns

a) Unsafe Type Casting
frontend/src/components/json/index.tsx:667

const Comp = as as 'input';

This forces the type from 'input' | 'textarea' to always be 'input', even when as === 'textarea'. This could cause runtime issues if textarea-specific props are passed. Consider:

const Comp = as; // Let TypeScript handle the union type properly

b) Missing Error Handling in useAsyncMemo
frontend/src/components/hooks/use-async-memo.tsx:11-15

promise.then((val) => {
  if (!cancel) {
    setVal(val)
  }
})

No .catch() handler for rejected promises. Failed promises will be unhandled. Consider:

promise.then((val) => {
  if (!cancel) {
    setVal(val)
  }
}).catch((error) => {
  console.error('useAsyncMemo error:', error);
  // Consider setting an error state or re-throwing
})

3. Performance Considerations

a) Unstable Callback Dependencies
frontend/src/components/actors/queries/actor.ts:91

const stableOnMessage = useCallback(onMessage, []); // ❌ onMessage not in deps

The dependency array is empty but should include onMessage. This could lead to stale closures. However, I see this might be intentional to avoid reconnections. Consider documenting this decision or use useEvent if available.

b) Missing Dependency Array
frontend/src/components/actors/queries/actor.ts:94-119
The useEffect dependency array is missing. It should include all dependencies or explicitly be empty:

}, [opts.enabled, url, stableOnMessage, actorId, queries])

📝 Minor Suggestions

1. Package.json Workspace Reference
   The change from "rivetkit": "*" to "rivetkit": "workspace:*" is good, but ensure this aligns with pnpm workspace best practices mentioned in CLAUDE.md.

2. Unused Imports
   Several files have unused imports that were added but not used:

   • frontend/src/app/data-providers/default-data-provider.tsx:3 - UseInfiniteQueryOptions (imported but not used)
   • frontend/src/app/data-providers/engine-data-provider.tsx:7 - QueryOptions (imported but not used)

3. Type Assertion Patterns
   Consider creating a helper type for query keys to avoid repetitive as QueryKey assertions:

   type TypedQueryKey<T extends readonly unknown[]> = T & QueryKey;

4. Missing EOF Newline
   .github/workflows/fe-pull-request.yml:35 - Missing newline at end of file (though this is minor)

🔒 Security Considerations

• No security issues identified
• Proper sanitization of headers in createClient function
• Good use of abort signals for request cancellation

🧪 Test Coverage

• The PR focuses on type safety, not behavioral changes
• Recommend running the full test suite to ensure no regressions
• Consider adding tests for the new useAsyncMemo hook

✨ Recommendations

Must Fix:

1. Remove namespaceId reference on line 193 of engine-data-provider.tsx
2. Add error handling to useAsyncMemo

Should Fix:
3. Fix the type casting in the Editable component
4. Add proper dependency arrays to useStream hook

Nice to Have:
5. Remove unused imports
6. Document intentional empty dependency arrays
7. Consider extracting the query key type pattern to reduce boilerplate

Summary

Great work on re-enabling TypeScript checking! The type safety improvements are valuable. The one critical bug (undefined namespaceId) must be fixed before merging. The other issues are mostly minor but worth addressing for code quality.

Score: 8/10 - Solid improvements with one blocking issue

---

🤖 Generated with Claude Code

[claude]

Pull Request Review: fix(fe): bring back ts type check

Summary

This PR re-enables TypeScript type checking in the frontend CI pipeline and fixes numerous type errors throughout the codebase. The changes primarily involve adding proper type annotations, fixing type mismatches, and introducing a new useAsyncMemo hook to handle async operations in React.

---

✅ Positive Aspects

1. CI/CD Improvement: Adding TypeScript checking to the CI pipeline is excellent for preventing type regressions
2. Comprehensive Type Fixes: The PR addresses type errors across many files systematically
3. React 19 Types: Upgrading to @types/react v19 shows forward compatibility
4. New Utility Hook: The useAsyncMemo hook is a useful addition for handling async memoization

---

🐛 Issues & Concerns

Critical: Missing Error Handling in useAsyncMemo

Location: frontend/src/components/hooks/use-async-memo.tsx:11-15

promise.then((val) => {
  if (!cancel) {
    setVal(val)
  }
})

Issue: No error handling for rejected promises. If the async factory throws or rejects, this will result in an unhandled promise rejection.

Fix:

promise.then((val) => {
  if (!cancel) {
    setVal(val)
  }
}).catch((error) => {
  // Handle or at least log the error
  console.error('useAsyncMemo error:', error)
})

---

Code Quality: Type Casting Instead of Proper Typing

Location: frontend/src/components/json/index.tsx:667

const Comp = as as 'input';

Issue: Type assertion bypasses TypeScript's type checking. This defeats the purpose of having types.

Recommendation: Properly type the component to handle both input and textarea without casting, or use a more robust solution:

type EditableElement = HTMLInputElement | HTMLTextAreaElement;
const ref = useRef<EditableElement | null>(null);

---

Code Quality: Redundant Null Check

Location: frontend/src/components/actors/queries/actor.ts:94-105

const controller = new AbortController();
if (!opts.enabled || !url) {
  controller.abort();
  return () => controller.abort();
}

async function establishConnection() {
  if(!url){  // <-- Redundant check
    return;
  }

Issue: The URL is already checked at line 97, so the check at line 103 is redundant.

Fix: Remove the redundant check inside establishConnection.

---

Code Quality: Missing Formatting

Location: frontend/src/components/actors/queries/actor.ts:94-95

useEffect(() => {
  // <-- Extra blank line here
  const controller = new AbortController();

Issue: Inconsistent formatting with an extra blank line.

---

Type Safety: Unsafe Type Assertions

Location: Multiple files with as QueryKey assertions

Examples:

• frontend/src/app/data-providers/default-data-provider.tsx:79
• frontend/src/app/data-providers/cloud-data-provider.tsx:375

Issue: While these fix the type errors, they're essentially telling TypeScript "trust me" instead of properly typing the query keys. This could hide actual type mismatches.

Recommendation: Consider defining proper query key types or using TanStack Query's type helpers.

---

Missing Validation: Space Formatting

Location: frontend/src/components/actors/queries/actor.ts:103

if(!url){  // Missing space after 'if'

Issue: Inconsistent with standard JavaScript/TypeScript formatting conventions.

---

🔒 Security Considerations

No major security issues identified. The changes are primarily type-related fixes.

---

⚡ Performance Considerations

1. useAsyncMemo Hook: The hook recreates the effect on every dependency change, which is expected. However, users should be careful with dependency arrays to avoid unnecessary re-executions.

2. Multiple Await Calls: In useStream, there are now two await calls:

   headers: await getHeaders(
     (await queries.createActorInspectorFetchConfiguration(actorId))
       ?.headers || {},
   ),

   This is sequential awaiting which could impact performance. Consider whether these could be parallelized or if the result could be cached.

---

🧪 Test Coverage

Missing: No tests added for the new useAsyncMemo hook. Given that it handles async operations and cancellation, it should have unit tests covering:

• Basic async resolution
• Cancellation when dependencies change
• Cleanup on unmount
• Error cases

---

📝 Recommendations

1. Add error handling to useAsyncMemo (critical)
2. Add unit tests for useAsyncMemo
3. Remove type assertions where possible and use proper typing
4. Clean up redundant null checks and formatting issues
5. Add JSDoc comments to useAsyncMmemo explaining its purpose and usage
6. Consider adding a cleanup function return from the factory in useAsyncMemo for more complex async operations

---

📊 Overall Assessment

Approval Status: ⚠️ Approve with Recommendations

The PR achieves its goal of re-enabling TypeScript checking and fixing type errors. However, there are some code quality issues and a critical missing error handler in the new hook. These should be addressed before merging to ensure code quality and reliability.

The CI/CD improvements are valuable and will help maintain type safety going forward.

---

🤖 Generated with Claude Code

[claude]

Pull Request Review: TypeScript Type Check Restoration

Summary

This PR successfully brings back TypeScript type checking to the frontend build process and resolves numerous type safety issues. The changes are well-structured and represent a significant improvement to code quality.

Positive Aspects

1. CI/CD Improvements ✅

• Added TypeScript check job to .github/workflows/fe-pull-request.yml
• Properly configured with pnpm cache and dependencies
• Will catch type errors before merge

2. Type Safety Enhancements ✅

• QueryKey type annotations: All query keys now properly typed with as QueryKey
• Explicit return types: Added return type annotations to query options (e.g., UseQueryOptions<string>)
• React 19 types: Upgraded to @types/react@^19 and @types/react-dom@^19
• Proper type imports: Added necessary imports like QueryKey, QueryOptions, UseQueryOptions, UseInfiniteQueryOptions

3. Async Handling Improvements ✅

• Created useAsyncMemo hook: Elegant solution for handling async operations in React hooks
• Proper async/await: Fixed functions that were returning promises synchronously
• Null safety: Added proper null/undefined checks (e.g., state || {})

Issues & Concerns

🔴 Critical: Error Handling in useAsyncMemo

File: frontend/src/components/hooks/use-async-memo.tsx:11-14

promise.then((val) => {
  if (!cancel) {
    setVal(val)
  }
})

Issue: Missing error handling for rejected promises. If the factory throws or returns a rejected promise, this will result in an unhandled promise rejection.

Recommendation:

promise.then((val) => {
  if (!cancel) {
    setVal(val)
  }
}).catch((error) => {
  if (!cancel) {
    console.error('useAsyncMemo error:', error)
    // Consider: setVal(initial) or setVal(undefined)
  }
})

🟡 Medium: Cleanup Function Missing

File: frontend/src/components/hooks/use-async-memo.tsx:7-19

Issue: The useEffect dependencies array (deps) is passed directly without being spread, which might cause issues with React's dependency comparison.

Current:

}, deps)

Should be:

// eslint-disable-next-line react-hooks/exhaustive-deps
}, deps)

Add the eslint disable comment since we're intentionally using the deps array as-is (this is a standard pattern for custom hooks that accept dependency arrays).

🟡 Medium: Type Narrowing in getChildIdx

File: frontend/src/components/code.tsx:49

const getChildIdx = (child: CodeGroupProps['children'][number]) =>

Issue: The type is too narrow - it assumes the child is specifically from CodeGroupProps children array. This works but could be more generic.

Recommendation: Consider using ReactElement<any> for more flexibility, similar to the mdx/code.tsx pattern.

🟢 Minor: Inconsistent Type Assertions

File: Multiple query files

Observation: Some type assertions use explicit return types (e.g., : UseQueryOptions<string>), while others use as QueryKey on the queryKey property. Both are valid, but consistency would be ideal.

Recommendation: Document in code comments or team guidelines which pattern to prefer.

🟢 Minor: Missing Cleanup Return

File: frontend/src/components/actors/queries/actor.ts:93

useEffect(() => {
  const controller = new AbortController();
  if (!opts.enabled || !url) {
    controller.abort();
    return () => controller.abort();
  }

Observation: Early return provides cleanup function, but the establishConnection() path below also needs to ensure cleanup. The current code seems fine, but verify that all code paths return cleanup functions.

🟢 Minor: Whitespace/Formatting

File: frontend/src/components/hooks/use-async-memo.tsx

Missing newline at end of file (line 21).

File: .github/workflows/fe-pull-request.yml:35

Missing newline at end of file.

Security Considerations

✅ No security concerns identified. The changes are primarily type-related and don't introduce new attack vectors.

Performance Considerations

✅ Good: Using mutationOptions() wrapper function instead of plain objects ensures proper type inference without runtime overhead.

⚠️ Minor concern: The useAsyncMemo hook will create new promises on every render if factory is not memoized. Consider documenting that users should wrap their factory in useCallback when necessary.

Test Coverage

❌ Missing: No tests added for the new useAsyncMemo hook.

Recommendation: Add unit tests covering:

• Basic async operation
• Cleanup on unmount
• Cleanup on dependency change
• Error handling (once added)
• Initial value behavior

Example test structure:

describe('useAsyncMemo', () => {
  it('should handle successful async operations', async () => { /* ... */ })
  it('should cleanup on unmount', () => { /* ... */ })
  it('should handle errors gracefully', () => { /* ... */ })
  it('should use initial value before promise resolves', () => { /* ... */ })
})

Code Quality & Best Practices

✅ Strengths:

1. Consistent patterns: QueryKey annotations applied uniformly
2. Type overloads: Good use of function overloads in useAsyncMemo
3. Null safety: Added defensive checks like state || {}
4. Removed unused imports: Cleaned up imports throughout

📋 Suggestions:

1. Consider adding JSDoc comments to useAsyncMemo explaining its purpose and usage
2. Add examples of proper usage in components that use async operations
3. Document the pattern for other developers

Conclusion

Overall Assessment: ✅ Approve with recommendations

This is a solid PR that significantly improves type safety in the frontend codebase. The core changes are well-implemented, and the addition of CI checks will prevent future type regressions.

Must address before merge:

• Add error handling to useAsyncMemo

Should address before merge:

• Add tests for useAsyncMemo
• Add newlines at end of files

Nice to have:

• Add JSDoc documentation
• Document type annotation patterns in team guidelines

Great work on bringing back type checking! 🎉

[claude]

PR Review: Bring Back TypeScript Type Check

Summary

This PR re-enables TypeScript type checking in the frontend CI pipeline and fixes numerous type issues throughout the codebase. Overall, this is a valuable improvement that strengthens type safety. However, there are several issues that need attention.

---

Critical Issues

1. Missing Error Handling in useAsyncMemo Hook

Location: frontend/src/components/hooks/use-async-memo.tsx:11-14

The new useAsyncMemo hook lacks error handling for rejected promises:

promise.then((val) => {
  if (!cancel) {
    setVal(val)
  }
})

Issue: If the promise rejects, the error will be unhandled and could cause issues in production.

Recommendation: Add a .catch() handler:

promise.then((val) => {
  if (!cancel) {
    setVal(val)
  }
}).catch((error) => {
  console.error('useAsyncMemo error:', error)
  // Consider calling an error boundary or setting error state
})

---

2. Redundant Null Check in useStream

Location: frontend/src/components/actors/queries/actor.ts:97-105

if (!opts.enabled || !url) {
  controller.abort();
  return () => controller.abort();
}

async function establishConnection() {
  if(!url){  // ← Redundant check
    return;
  }
  // ...
}

Issue: The url check at line 103 is redundant since we already check it at line 97.

Recommendation: Remove the inner check or add a comment explaining why it's necessary.

---

3. Inconsistent Code Style

Location: frontend/src/components/hooks/use-async-memo.tsx

The new file doesn't follow the project's formatting conventions:

• Missing spaces after { and before } in imports
• Inconsistent spacing around keywords

Example:

import {DependencyList, useEffect, useState} from 'react'  // Missing spaces

Recommendation: Run the code formatter (though CLAUDE.md says not to run cargo fmt automatically, this is a TypeScript file, so Biome should handle it).

---

Moderate Issues

4. Type Assertions Could Be More Specific

Location: Multiple files (e.g., frontend/src/app/data-providers/engine-data-provider.tsx)

Many query keys are cast to QueryKey type:

queryKey: ["actor", actorId] as QueryKey,

Issue: While this fixes type errors, it bypasses TypeScript's type checking for query keys.

Recommendation: Consider defining proper types for query keys rather than using type assertions. This would provide better type safety throughout the codebase.

---

5. Potential Race Condition in Stream Reconnection

Location: frontend/src/components/actors/queries/actor.ts:116-119

onclose: async () => {
  await new Promise((resolve) => setTimeout(resolve, 1000));
  controller.signal.throwIfAborted();
  establishConnection();
},

Issue: If the component unmounts during the 1-second delay, establishConnection() could still be called after cleanup.

Recommendation: Check the cancellation flag before reconnecting:

onclose: async () => {
  await new Promise((resolve) => setTimeout(resolve, 1000));
  if (controller.signal.aborted) return;
  establishConnection();
},

---

6. Unsafe Nullish Coalescing

Location: Multiple locations in actor filters and list components

select: (state) => fn(pick(state || {})),

Issue: While this fixes type errors, it masks potential bugs where state might legitimately be undefined due to routing issues.

Recommendation: Investigate why state can be undefined and handle it more explicitly, possibly with early returns or default values at a higher level.

---

Minor Issues

7. Unused Type Import

Location: frontend/src/app/data-providers/cloud-data-provider.tsx:3

import { infiniteQueryOptions, QueryKey, QueryOptions, queryOptions, UseQueryOptions } from "@tanstack/react-query";

Issue: QueryOptions appears to be imported but not used in this file.

Recommendation: Remove unused imports to keep the code clean.

---

8. Type Assertion Could Be Avoided

Location: frontend/src/components/json/index.tsx:664

const Comp = as as 'input';

Issue: This double type assertion is a code smell that suggests the typing could be improved.

Recommendation: Consider refactoring to avoid the need for this assertion, perhaps by using function overloads or conditional types.

---

9. Missing Space in Formatting

Location: frontend/src/app/data-providers/engine-data-provider.tsx:542

mutationKey: ["runner-config"]  as QueryKey,

Issue: Extra space before as.

---

Positive Aspects

1. CI Enhancement: Adding TypeScript checking to the CI pipeline is excellent for catching type errors early.

2. Comprehensive Type Fixes: The PR addresses type issues across many files systematically.

3. React 19 Types: Upgrading to @types/react@19 keeps the project up-to-date with the latest React type definitions.

4. Custom Hook Solution: The useAsyncMemo hook is a reasonable solution for handling async operations in useMemo-like scenarios.

---

Recommendations

1. Add error handling to useAsyncMemo hook (Critical)
2. Remove redundant null checks to improve code clarity
3. Run Biome formatter on the new hook file
4. Consider a more robust query key typing strategy instead of widespread type assertions
5. Add tests for the new useAsyncMemo hook to ensure it handles edge cases correctly
6. Document the async patterns - Consider adding JSDoc comments to explain when and why useAsyncMemo should be used

---

Test Coverage

Concern: This PR adds a new custom hook (useAsyncMemo) but doesn't include tests for it. Given that it handles async operations and cancellation, it would benefit from unit tests covering:

• Promise resolution
• Promise rejection
• Cleanup/cancellation
• Re-execution on dependency changes

---

Overall Assessment

This is a good PR that improves type safety in the codebase. The issues identified are mostly minor, with the exception of the missing error handling in useAsyncMemo. Once the critical issue is addressed and the formatting is cleaned up, this should be ready to merge.

Recommendation: Request changes to address the error handling issue before merging.

[claude]

Code Review - PR #3259: Bring Back TS Type Check

Summary

This PR re-enables TypeScript type checking in the CI pipeline and fixes numerous type errors throughout the frontend codebase. Overall, this is a positive change that improves type safety, but there are several issues that need to be addressed.

---

Critical Issues

1. Missing Error Handling in useAsyncMemo Hook

Location: frontend/src/components/hooks/use-async-memo.tsx:11-15

The new useAsyncMemo hook doesn't handle promise rejections, which could lead to unhandled promise rejection errors:

promise.then((val) => {
  if (!cancel) {
    setVal(val)
  }
})

Recommendation: Add error handling:

promise.then((val) => {
  if (!cancel) {
    setVal(val)
  }
}).catch((error) => {
  if (!cancel) {
    console.error('useAsyncMemo error:', error)
    // Consider: setVal(initial) or expose error state
  }
})

2. Unsafe Type Assertions with as QueryKey

Location: Multiple files in data providers

The PR adds many type assertions like queryKey: ["actors", opts] as QueryKey throughout the codebase. While this fixes type errors, it bypasses type safety.

Example from default-data-provider.tsx:79:

queryKey: ["actors", opts] as QueryKey,

Concern: If QueryKey expectations change in TanStack Query, these assertions could hide type mismatches.

Recommendation: Consider defining proper type-safe query key factories instead of using type assertions everywhere.

---

Major Issues

3. Removed namespaceId Parameter

Location: frontend/src/app/data-providers/engine-data-provider.tsx:134

export const createNamespaceContext = ({
  namespace,
- namespaceId,
  client,

The namespaceId parameter was removed but it's unclear if this is intentional or if it could break existing functionality.

Question: Is namespaceId no longer needed? Should this be documented in the commit message?

4. Unsafe pick(state || {}) Pattern

Location: frontend/src/components/actors/actor-filters-context.tsx:90

select: (state) => fn(pick(state || {})),

This pattern suggests that state might be undefined, but falling back to an empty object might not be the right behavior.

Recommendation: Investigate why state could be undefined and handle it more explicitly.

---

Minor Issues

5. Missing Newline at End of File

Location: .github/workflows/fe-pull-request.yml:35

The workflow file is missing a trailing newline (should end with \n).

6. Code Formatting Issues in use-async-memo.tsx

Location: frontend/src/components/hooks/use-async-memo.tsx

The new hook doesn't follow the codebase's formatting conventions:

• Missing spaces after colons in function signatures
• Inconsistent spacing

Expected (based on CLAUDE.md):

export function useAsyncMemo<T>(
  factory: () => Promise<T> | undefined | null,
  deps: DependencyList
): T | undefined

7. Unnecessary Type Assertion in json/index.tsx

Location: frontend/src/components/json/index.tsx:667

const Comp = as as 'input';

This double type assertion looks suspicious. Consider refactoring to avoid this pattern.

---

Positive Changes

✅ Re-enabled TypeScript checking in CI - Great addition for preventing type regressions
✅ Upgraded to React 19 types - Keeps dependencies up to date
✅ Fixed numerous type errors - Improves overall code quality
✅ Added proper return types - e.g., engineAdminTokenQueryOptions(): UseQueryOptions<string>
✅ Improved null safety - Many changes add proper undefined checks

---

Recommendations

1. High Priority: Add error handling to useAsyncMemo hook
2. High Priority: Review all as QueryKey assertions and consider a more type-safe approach
3. Medium Priority: Verify that removing namespaceId parameter doesn't break functionality
4. Low Priority: Fix formatting in use-async-memo.tsx to match project conventions
5. Low Priority: Add trailing newline to workflow file

---

Test Coverage

The PR doesn't include any new tests for the useAsyncMemo hook. Consider adding:

• Tests for successful async resolution
• Tests for promise rejection handling
• Tests for cleanup on unmount
• Tests for dependency changes

---

Security Concerns

No significant security issues identified. The changes are primarily type-related improvements.

---

Overall Assessment: This PR makes valuable improvements to type safety, but needs refinement before merging. The missing error handling in useAsyncMemo and widespread use of type assertions are the main concerns.

🤖 Generated with Claude Code

[graphite-app]

Merge activity

• Oct 25, 9:24 AM UTC: Graphite rebased this pull request, because this pull request is set to merge when ready.
• 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.

[claude]

Pull Request Review: Re-enable TypeScript Type Checking

Overview

This PR successfully re-enables TypeScript type checking in the frontend CI pipeline and addresses the type errors that were preventing it from running. The changes upgrade to React 19 types and add proper type annotations throughout the codebase.

---

✅ Positive Observations

1. CI/CD Improvements

• GitHub Actions: Added dedicated TypeScript compiler check in CI workflow
• Path updates: Correctly updated monitored paths to include engine/sdks/typescript/** and rivetkit-typescript/**
• Working directory: Properly scoped commands to ./frontend

2. Type Safety Enhancements

• Extensive use of explicit QueryKey type assertions improves type safety
• Added proper generic type parameters to query options
• Fixed unsafe type coercions throughout the codebase

3. React 19 Upgrade

• Updated @types/react and @types/react-dom to v19
• Addressed breaking changes in React types

---

🔍 Issues & Concerns

CRITICAL: Missing Error Handling in useAsyncMemo

Location: frontend/src/components/hooks/use-async-memo.tsx:11-15

promise.then((val) => {
  if (!cancel) {
    setVal(val)
  }
})

Issue: The promise has no .catch() handler, which means unhandled promise rejections will be silently swallowed or bubble up as unhandled rejections.

Impact:

• Debugging will be extremely difficult
• Application could fail silently
• React error boundaries won't catch these errors properly

Recommendation:

promise.then((val) => {
  if (!cancel) {
    setVal(val)
  }
}).catch((error) => {
  if (!cancel) {
    console.error('useAsyncMemo error:', error)
    // Consider: throw error or setVal to error state
  }
})

BUG: Undefined Variable Reference

Location: frontend/src/app/data-providers/engine-data-provider.tsx:193

queryKey: [
  { namespace, namespaceId },  // ❌ namespaceId is not defined
  ...def.regionQueryOptions(regionId).queryKey,
],

Issue: namespaceId was removed from the function parameters (line 135) but is still referenced here.

Impact: This will cause a runtime error when regionQueryOptions is called.

Fix: Remove namespaceId from the query key:

queryKey: [
  { namespace },
  ...def.regionQueryOptions(regionId).queryKey,
],

Type Safety Concerns

1. Unsafe Type Assertions

Multiple instances of as QueryKey assertions throughout the codebase:

• frontend/src/app/data-providers/default-data-provider.tsx:79, 107, 165, 223, 230, 233, 252, 270, 322, 334, 344, 355
• frontend/src/app/data-providers/engine-data-provider.tsx (numerous locations)

While this fixes the immediate type errors, it bypasses TypeScript's type checking. Consider if TanStack Query's types can be better aligned with your usage patterns.

2. Unsafe Nullability Handling

Location: frontend/src/components/actors/actor-filters-context.tsx:90

select: (state) => fn(pick(state || {})),

Issue: If state is null/undefined, pick({}) may not match the expected type contract of fn.

Recommendation: Validate the actual type of state and handle the null case explicitly rather than defaulting to an empty object.

3. Component Type Assertions

Location: frontend/src/components/modal-renderer.tsx:46-49

return dialog.Dialog as ComponentType<{
  dialogProps?: DialogProps;
  dialogContentProps?: ComponentProps<typeof DialogContent>;
}>;

Issue: Type assertion that may not reflect runtime reality. If the component doesn't match this signature, you'll get runtime errors.

---

🎯 Recommendations

High Priority

1. Fix the namespaceId bug in engine-data-provider.tsx:193
2. Add error handling to useAsyncMemo hook
3. Add unit tests for the new useAsyncMemo hook to ensure:
   • Cancellation works correctly
   • Error cases are handled
   • Memory leaks don't occur

Medium Priority

4. Review type assertions: Consider if there's a better way to satisfy TypeScript without as QueryKey everywhere
5. Add JSDoc comments to useAsyncMemo explaining:
   • When to use it vs regular useMemo
   • Cleanup behavior
   • Error handling expectations

Low Priority

6. Code formatting: The new use-async-memo.tsx file lacks consistent formatting:
   • Missing spaces in function signatures
   • Consider running the formatter on new files

---

📊 Test Coverage

Missing: No test files were added for:

• useAsyncMemo hook (should be tested for edge cases)
• Modified query options (integration tests recommended)

Recommendation: Add tests for the new async memo hook, especially around:

• Cancellation behavior
• Error states
• Memory leaks with rapid re-renders

---

🔒 Security Considerations

No significant security concerns identified. The changes are primarily type-system improvements.

---

⚡ Performance Considerations

Positive:

• Type checking in CI will catch errors earlier
• Query key type safety reduces runtime errors

Concern:

• useAsyncMemo creates a new promise on every render when deps change. Ensure this is intentional and won't cause performance issues with frequently changing dependencies.

---

📝 Summary

This PR makes meaningful progress toward type safety in the frontend. However, there are two critical issues that must be addressed before merging:

1. Fix the namespaceId undefined variable bug
2. Add error handling to useAsyncMemo

Once these are resolved, the PR will be in good shape. Consider adding tests for the new hook to prevent regressions.

Overall Assessment: ⚠️ Needs Changes - Good direction but critical bugs must be fixed first.