Skip to content

fix(frontend): navigate only by using actor ids#5023

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

fix(frontend): navigate only by using actor ids#5023
jog1t merged 1 commit into
mainfrom
05-05-fix_frontend_navigate_only_by_using_actor_ids

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

@railway-app

railway-app Bot commented May 11, 2026

Copy link
Copy Markdown

🚅 Deployed to the rivet-pr-5023 environment in rivet-frontend

Service Status Web Updated (UTC)
kitchen-sink ❌ Build Failed (View Logs) Web May 15, 2026 at 7:37 pm
frontend-cloud 🕒 Building (View Logs) Web May 15, 2026 at 7:37 pm
ladle 🕒 Building (View Logs) Web May 15, 2026 at 7:37 pm
frontend-inspector 🕒 Building (View Logs) Web May 15, 2026 at 7:37 pm
website 😴 Sleeping (View Logs) Web May 11, 2026 at 6:56 pm
mcp-hub ✅ Success (View Logs) Web May 11, 2026 at 6:40 pm

@claude

claude Bot commented May 11, 2026

Copy link
Copy Markdown
Contributor

PR Review: fix(frontend): navigate only by using actor ids

Overview

This PR simplifies the actor navigation model by removing the dual actorKey/actorId URL parameter approach and consolidating everything to use only actorId. The core idea is sound: instead of letting a raw actor key propagate through URL state, resolve it to an ID upfront and navigate with that. It also fixes a real bug where the route loader was reading .key instead of .actorId when selecting the first actor.


Bug Fix (Correct)

In both route loaders (ns.$namespace/index.tsx), the code was reading:

const firstActorId = actors.pages[0]?.actors?.[0]?.key;

This was using the actor's key (a string tag) as the navigation ID. The fix to .actorId is correct and important.


Issues

1. Silent catch {} in go-to-actor-button.tsx

try {
    await queryClient.fetchQuery(dataProvider.actorQueryOptions(trimmed));
    // ...
    return;
} catch {}

An empty catch block swallows all errors silently, including unexpected ones (network failures, malformed responses, etc.). The intent is to fall through to the key-based lookup when an ID isn't found, but the current form also masks real errors. Consider only catching a specific "not found" condition, or at minimum logging in dev mode.

2. Potentially expensive parallel requests in go-to-actor-button.tsx

const resolved = await Promise.any(
    buildNames.map((name) =>
        queryClient.fetchQuery(
            dataProvider.actorQueryOptions({ key: trimmed, name }),
        ),
    ),
).catch(() => null);

This fires one request per build name simultaneously. If a project has many builds, this triggers N parallel fetches for the key-resolution fallback. For an explicit user action this may be acceptable, but a comment explaining why Promise.any is used here (first matching build name wins) and whether the result set is expected to be small would help future readers.

3. Leftover n in actors-list.tsx

const { actorId, n } = useSearch({ from: '/_context' });

n is still destructured but is no longer used in the isCurrent comparison, which was the only place it mattered in this component. If it isn't used elsewhere in the full component body, this is dead code worth removing.

4. Promise.any([]) edge case

When buildNames is empty, Promise.any([]) immediately rejects with an AggregateError. The .catch(() => null) handles this correctly, and the toast message appears. This is acceptable behavior but non-obvious — a short comment would clarify intent.


Positive Changes

  • Removing actorKey from URL state is a clean simplification. Keyed lookup now happens at the call site (GoToActorButton) and the resolved UUID is what flows through routing state.
  • ActorNotFound, ActorsListRow, and Actor are all meaningfully simpler.
  • The toast error message in go-to-actor-button.tsx is a UX improvement over the previous silent fallback navigation to a potentially invalid key.
  • Both route variants (ns.$namespace and orgs.$organization/projects.$project/ns.$namespace) are updated consistently.

Summary

The direction is right and the core bug fix (.key.actorId) is valuable. The main concerns are the silent empty catch, potentially unbounded parallel requests, and the leftover n destructure. None are blocking given the PR is already merged, but worth a follow-up.

@jog1t jog1t force-pushed the 04-29-feat_frontend_add_serverless_check_when_editing_the_runner_config branch from c92292f to 1fd52f8 Compare May 11, 2026 18:49
@jog1t jog1t force-pushed the 05-05-fix_frontend_navigate_only_by_using_actor_ids branch from 339b52a to 22e8455 Compare May 11, 2026 18:49
@railway-app railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5023 May 11, 2026 18:49 Destroyed
@jog1t jog1t force-pushed the 05-05-fix_frontend_navigate_only_by_using_actor_ids branch from 22e8455 to a29da76 Compare May 15, 2026 19:30
@railway-app railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5023 May 15, 2026 19:30 Destroyed
@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:38 PM UTC: Graphite rebased this pull request as part of a merge.
  • May 15, 7:38 PM UTC: @jog1t merged this pull request with Graphite.

@jog1t jog1t changed the base branch from 04-29-feat_frontend_add_serverless_check_when_editing_the_runner_config to graphite-base/5023 May 15, 2026 19:36
@jog1t jog1t changed the base branch from graphite-base/5023 to main May 15, 2026 19:37
@jog1t jog1t force-pushed the 05-05-fix_frontend_navigate_only_by_using_actor_ids branch from a29da76 to f4118ca Compare May 15, 2026 19:37
@railway-app railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5023 May 15, 2026 19:37 Destroyed
@jog1t jog1t merged commit 306e736 into main May 15, 2026
7 of 14 checks passed
@jog1t jog1t deleted the 05-05-fix_frontend_navigate_only_by_using_actor_ids branch May 15, 2026 19:38
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