-
Notifications
You must be signed in to change notification settings - Fork 142
feat(dashboard): editable serverless configs #3765
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
How to use the Graphite Merge QueueAdd 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 Automations"Test" took an action on this PR • (01/07/26)1 assignee was added to this PR based on Kacper Wojciechowski's automation. |
PR Review: Editable Serverless ConfigsI've reviewed PR #3765 and have several observations and recommendations: Code Quality Issues1. Broken Logic in SharedSettingsForm (edit-runner-config.tsx:54-56)const isSharedSettings = Object.values(data?.datacenters || {}).every(
(dcConfig) => JSON.stringify(dcConfig) === JSON.stringify(dcConfig),
);This comparison always returns const dcConfigs = Object.values(data?.datacenters || {});
const isSharedSettings = dcConfigs.length > 0 && dcConfigs.every(
(dcConfig) => JSON.stringify(dcConfig) === JSON.stringify(dcConfigs[0]),
);2. Strict Equality vs Loose Equality (edit-runner-config.tsx:86)Inconsistent comparison operators:
Should use strict equality consistently. 3. Unused Import (edit-runner-config.tsx:25)
4. Type Safety Issue (edit-single-runner-config-form.tsx:27-28)export const Enable = <TValues extends Record<string, any> = FormValues>({name}: {name: FieldPath<TValues>;}) => {
const { control } = useFormContext<FormValues>();The generic parameter const { control } = useFormContext<TValues>();5. Weak Type Assertion (edit-single-runner-config-form.tsx:37)checked={field.value as unknown as boolean ?? false}Double casting suggests a type mismatch. Consider fixing the underlying type issue rather than forcing it. 6. Formatting Issue (edit-runner-config.tsx:311)Extra whitespace before closing tag: <EditSingleRunnerConfigForm.Datacenters/>Potential Bugs7. Data Mutation Risk (edit-runner-config.tsx:259){
...data.datacenters[dc.id]?.serverless || defaultServerlessConfig,
enable: !!data.datacenters[dc.id]?.serverless,
// ...
}The spread operator precedence means if ...(data.datacenters[dc.id]?.serverless || defaultServerlessConfig)8. Unhandled Edge Case (edit-runner-config.tsx:147-152)If all datacenters have Performance Considerations9. Multiple Query Calls
10. Inefficient Object Iteration (edit-runner-config.tsx:274)for (const [dcId, dcConfig] of Object.entries(values.datacenters || {})) {Consider filtering disabled datacenters before iteration for better performance with many regions. Security Concerns✅ No major security issues detected. Headers are properly handled and user input is validated through Zod schemas. Best Practices11. Inconsistent Error HandlingNo error boundaries or loading states for mutation failures. Consider adding error handling to 12. Magic Numbers (edit-runner-config.tsx:28-35)Default values like 13. Code DuplicationQuery invalidation is repeated in multiple places: await queryClient.invalidateQueries(provider.runnerConfigsQueryOptions());
await queryClient.invalidateQueries(provider.runnerConfigQueryOptions({ name }));Consider extracting to a helper function. Test CoveragePositive Aspects
RecommendationsCritical:
High Priority: Medium Priority: Low Priority: Let me know if you'd like me to help fix any of these issues! |
851656c to
f23273a
Compare
06d1b43 to
69729dc
Compare

TL;DR
Refactored runner configuration UI to support both shared and per-datacenter settings, improving the management of serverless providers across multiple regions.
What changed?
RunnerConfigToggleGroupcomponent for switching between serverless and dedicated modesHow to test?
Why make this change?
This change provides more flexibility in managing runner configurations across multiple regions. Users can now choose between applying the same configuration to all datacenters or customizing settings for specific regions. The improved UI makes it easier to identify and troubleshoot configuration issues with better error reporting and visualization of provider status across regions.