Skip to content

🧹 Fix Unsafe Redux State Mutation in projectsSlice#44

Open
sachasayan wants to merge 3 commits intomainfrom
fix-unsafe-mutation-projects-slice-16382191194992882127
Open

🧹 Fix Unsafe Redux State Mutation in projectsSlice#44
sachasayan wants to merge 3 commits intomainfrom
fix-unsafe-mutation-projects-slice-16382191194992882127

Conversation

@sachasayan
Copy link
Copy Markdown
Owner

The updateMetaProperty reducer in projectsSlice.ts previously used an unsafe as any cast to assign values to dynamic properties of the activeProject. This bypassed TypeScript's type checking and posed a risk of runtime errors.

I've refactored this to be fully type-safe by:

  1. Defining a ProjectMetaUpdate type that links each Project property key with its corresponding value type.
  2. Using this type in the updateMetaProperty action payload.
  3. implementing a switch statement in the reducer to narrow the types of both the property and value, allowing for safe assignment without casting.
  4. Adding a default case with a localized cast to ensure that any new properties added to Project in the future are still handled, preventing the reducer from becoming a restrictive whitelist.
  5. Ensuring that projectHasLiveEdits is only updated when a change is actually applied.

This improvement enhances the maintainability and reliability of the project's state management.


PR created automatically by Jules for task 16382191194992882127 started by @sachasayan

- Introduced `ProjectMetaUpdate` discriminated union for type-safe property updates.
- Refactored `updateMetaProperty` reducer to use a switch statement for type narrowing.
- Eliminated `as any` casting of `state.activeProject`.
- Added a fallback `default` case to ensure future-proofing.
- Fixed logic to only set `projectHasLiveEdits` if an update occurred.

Co-authored-by: sachasayan <1666034+sachasayan@users.noreply.github.com>
@google-labs-jules
Copy link
Copy Markdown
Contributor

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@sachasayan
Copy link
Copy Markdown
Owner Author

@jules failing ci

@google-labs-jules
Copy link
Copy Markdown
Contributor

@jules failing ci

I've fixed the regression in projectsSlice.ts where the default case was causing a TypeScript error on the never type. I also took the opportunity to fix a pre-existing TypeScript error in OnboardingPage.tsx that was blocking the build. pnpm typecheck and pnpm test are now passing.

- Fixed regression in `updateMetaProperty` where `default` case reached `never` type.
- Fixed pre-existing type error in `OnboardingPage.tsx` blocking CI.
- Simplified `updateMetaProperty` switch statement to be more concise while maintaining type safety for key properties.

Co-authored-by: sachasayan <1666034+sachasayan@users.noreply.github.com>
@sachasayan
Copy link
Copy Markdown
Owner Author

@jules rebase

@google-labs-jules
Copy link
Copy Markdown
Contributor

@jules rebase

I've rebased the branch onto the latest main. I resolved a conflict in projectsSlice.ts and confirmed that typecheck:web and all tests are passing. Note: There are some pre-existing type errors in src/main/index.ts (affecting typecheck:node) that seem to be on the base branch, but they are unrelated to my changes.

- Rebased onto latest main.
- Resolved conflict in projectsSlice.ts.
- Verified typecheck:web and tests pass.

Co-authored-by: sachasayan <1666034+sachasayan@users.noreply.github.com>
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