Skip to content

refactor(forntend): improve providers table#5033

Merged
jog1t merged 1 commit into
mainfrom
05-11-refactor_forntend_improve_providers_table
May 15, 2026
Merged

refactor(forntend): improve providers table#5033
jog1t merged 1 commit into
mainfrom
05-11-refactor_forntend_improve_providers_table

Conversation

@jog1t

@jog1t jog1t commented May 11, 2026

Copy link
Copy Markdown
Contributor

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@claude

claude Bot commented May 11, 2026

Copy link
Copy Markdown
Contributor

Code Review: refactor(frontend): improve providers table

Overview

This PR decouples RunnerConfigsTable from TanStack Router navigation and React Query data fetching, injecting those concerns as props (renderRegion, onEditConfig, onDeleteConfig, totalDatacenterCount). It also adds a ProviderSummary component showing provider + kind (serverless/runner) inline, improves the endpoint tooltip to show per-datacenter URLs, adds Ladle stories, and implements a redirect-after-login flow via the from query param.


Potential Bugs

totalDatacenterCount can be NaN - in settings.tsx:

select: (data) =>
  data.pages.reduce((acc, page) => acc + page.datacenters?.length, 0),

page.datacenters?.length returns undefined when datacenters is missing, so acc + undefined = NaN. The Regions component guards with totalDatacenterCount !== undefined && regions.length === totalDatacenterCount, and NaN === anything is always false - so the "Global" label silently fails to appear when it should. Fix:

data.pages.reduce((acc, page) => acc + (page.datacenters?.length ?? 0), 0)

Inconsistent open-redirect guard for Google OAuth callback - login.tsx uses an inline check:

from && from.startsWith("/") && !from.startsWith("//")

But isSafeInternalPath in auth.ts additionally blocks /login, /join, /verify-email-pending, and /forgot-password. After a Google OAuth round-trip, a from=/login param would pass the looser inline check and create a redirect loop. Consider exporting isSafeInternalPath and reusing it in login.tsx.

redirect({ to: from }) with a raw string - TanStack Router's to expects a typed route path. Passing an arbitrary string may produce a TypeScript error in strict mode. Using href: from (TanStack Router's escape hatch for raw paths) would be more explicit and avoids the typed-route constraint.


Code Quality

  • Good: The provider if/else chain replaced with PROVIDER_ICONS/PROVIDER_LABELS lookup tables is much cleaner and easier to extend.
  • Good: Decoupling navigation and data fetching from the table component makes it independently testable - the Ladle stories are a direct payoff.
  • Good: The endpoint tooltip now shows per-datacenter breakdown (region + URL) rather than a flat list of unique URLs, which is more useful when datacenters share the same endpoint.
  • Note: The Ping component is replaced with inline Tailwind animation markup. If Ping still exists in the design system, prefer the shared component to avoid visual drift over time.

Security

  • isSafeInternalPath correctly prevents protocol-relative URLs (//evil.com) and enforces same-origin paths. The auth-page blocklist is a nice touch to avoid loops.
  • The from -> redirect in redirectToOrganization fires before the org-lookup flow, meaning a valid from bypasses org selection entirely. Worth confirming: if from points to a resource that requires a specific org context, the org context may not be set when the user lands there.

Minor

  • PR title has a typo: "forntend" should be "frontend".
  • PR description checklist is empty - no test description or self-review recorded.

Overall this is a clean, well-structured refactor. The decoupling pattern is correct, and the stories cover the meaningful edge cases (empty endpoint set, mixed kinds, error state, global deployment). The ?? 0 NaN fix and the Google OAuth path-check inconsistency are the two items worth addressing.

@jog1t jog1t force-pushed the 05-11-fix_frontend_missing_provider_configuration_right_after_adding_it branch from f633f55 to c64f1b8 Compare May 11, 2026 18:49
@jog1t jog1t force-pushed the 05-11-refactor_forntend_improve_providers_table branch from 223bdc0 to 2b7c11e Compare May 11, 2026 18:49
@jog1t jog1t force-pushed the 05-11-fix_frontend_missing_provider_configuration_right_after_adding_it branch from c64f1b8 to e1f82ef Compare May 15, 2026 19:30
@jog1t jog1t force-pushed the 05-11-refactor_forntend_improve_providers_table branch from 2b7c11e to 3a3f4db Compare May 15, 2026 19:30
@jog1t jog1t mentioned this pull request May 15, 2026
11 tasks

jog1t commented May 15, 2026

Copy link
Copy Markdown
Contributor Author

Merge activity

  • May 15, 7:36 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • May 15, 7:58 PM UTC: Graphite rebased this pull request as part of a merge.
  • May 15, 7:58 PM UTC: @jog1t merged this pull request with Graphite.

@jog1t jog1t changed the base branch from 05-11-fix_frontend_missing_provider_configuration_right_after_adding_it to graphite-base/5033 May 15, 2026 19:55
@jog1t jog1t changed the base branch from graphite-base/5033 to main May 15, 2026 19:57
@jog1t jog1t force-pushed the 05-11-refactor_forntend_improve_providers_table branch from 3a3f4db to 15a9161 Compare May 15, 2026 19:57
@jog1t jog1t merged commit 6b45bb8 into main May 15, 2026
8 of 11 checks passed
@jog1t jog1t deleted the 05-11-refactor_forntend_improve_providers_table branch May 15, 2026 19:58
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