Skip to content

Conversation

@EamonNerbonne
Copy link
Contributor

@EamonNerbonne EamonNerbonne commented Jun 12, 2025

Description

Reduce duplication between compile-time types and run-time types. The aim is to reduce the amount of busywork when changing in particular settings state, without loss of compile-time type safety. This originates from Kilo-Org/kilocode#710 (where I work), but I hope it'll be useful for you too?

Pre-Submission Checklist

  • Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
  • Scope: My changes are focused on the linked issue (one major feature/fix per PR).

I think an example speaks more clearly than type-system mumbo jumbo, so I decided to propose this with this example. If that's really unwanted - no hard feelings for closing the PR.

  • Self-Review: I have performed a thorough self-review of my code.
  • Code Quality:
    • ? My code adheres to the project's style guidelines.
    • There are no new linting errors or warnings (npm run lint).
    • All debug code (e.g., console.log) has been removed.
  • Testing:
    • n/a New and/or updated tests have been added to cover my changes.
    • All tests pass locally (npm test).
    • The application builds successfully with my changes.
  • Branch Hygiene: My branch is up-to-date (rebased) with the main branch.
  • n/a Documentation Impact: I have considered if my changes require documentation updates (see "Documentation Updates" section below).
  • n/a Changeset: A changeset has been created using npm run changeset if this PR includes user-facing changes or dependency updates.
  • Contribution Guidelines: I have read and agree to the Contributor Guidelines.

Screenshots / Videos


main:
image

this branch:
image


main:
image

this branch:
image


One impact to be aware of is that the ordering of the string arrays now follows those of the zod types, and is thus no longer manual. I don't believe that's relevant, but perhaps I'm missing some subtlety.


Important

Refactor to reduce type duplication by using zod schema key extraction in global-settings.ts and provider-settings.ts.

  • Refactor:
    • Remove keysOf function from type-fu.ts.
    • Replace manual key arrays with zod schema key extraction in global-settings.ts and provider-settings.ts.
  • Behavior:
    • GLOBAL_SETTINGS_KEYS and PROVIDER_SETTINGS_KEYS now use zod schema key extraction.
    • SECRET_STATE_KEYS is now a constant array with type assertion.
  • Misc:
    • Ordering of string arrays now follows zod types, not manual.

This description was created by Ellipsis for ba44e63. You can customize this summary. It will automatically update as commits are pushed.

@EamonNerbonne EamonNerbonne requested review from cte, jr and mrubens as code owners June 12, 2025 17:22
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jun 12, 2025
@EamonNerbonne EamonNerbonne changed the title ♻️ **Refactor**: Avoid type system duplication ♻️ Refactor: Avoid type system duplication Jun 12, 2025
@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Jun 12, 2025
@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap Jun 12, 2025
@hannesrudolph hannesrudolph added PR - Needs Preliminary Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Jun 12, 2025
Copy link
Contributor

@hassoncs hassoncs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can do this and still get the same level of compile-time typing, I feel like there's no downside?

@EamonNerbonne
Copy link
Contributor Author

Yes, this should have the same can't-miss-anything compile-time typing safety guarrantees. Apart from the ordering, it's I think equivalent.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 13, 2025
@mrubens mrubens merged commit e535b55 into RooCodeInc:main Jun 13, 2025
25 checks passed
@github-project-automation github-project-automation bot moved this from PR [Needs Prelim Review] to Done in Roo Code Roadmap Jun 13, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Jun 13, 2025
@hannesrudolph
Copy link
Collaborator

@EamonNerbonne do you have discord?

@EamonNerbonne
Copy link
Contributor Author

yeah; I replied in discord for convenience.

@hannesrudolph
Copy link
Collaborator

@EamonNerbonne I am sorry I am not sure which person you are. Can you DM me (hrudolph)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer PR - Needs Preliminary Review size:L This PR changes 100-499 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants