Skip to content

Conversation

@MasterPtato
Copy link
Contributor

No description provided.

@vercel
Copy link

vercel bot commented Jan 7, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
rivet-cloud Ready Ready Preview, Comment Jan 7, 2026 1:17am
3 Skipped Deployments
Project Deployment Review Updated (UTC)
rivet-inspector Ignored Ignored Preview Jan 7, 2026 1:17am
rivet-site Ignored Ignored Preview Jan 7, 2026 1:17am
rivetkit-serverless Skipped Skipped Jan 7, 2026 1:17am

Copy link
Contributor Author

MasterPtato commented Jan 7, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add 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.

@claude
Copy link

claude bot commented Jan 7, 2026

PR Review: Remove method from key for consistency

Summary

This PR removes the actor_id() getter method from WorkflowIdKey and makes the actor_id field public instead. The change improves consistency with how similar key structs in the codebase expose their fields.

Positives

  1. Consistency: This change aligns WorkflowIdKey with other key structs in the same file (e.g., CreateTsKey, RunnerIdKey, NamespaceIdKey, etc.) which all have private fields without getter methods.

  2. Simplification: Removes unnecessary boilerplate - the getter method was only wrapping direct field access with no additional logic.

  3. Clean Migration: The only usage site was updated correctly in actor_runner_name_selector_backfill.rs:167,171, replacing key.actor_id() with direct field access key.actor_id.

  4. No Breaking Changes: A grep search confirms there are no other usages of the removed .actor_id() method in the codebase.

Observations

  1. Title Typo: The PR title has a typo - consistently should be consistency

  2. Inconsistency in File: Looking at the broader context, there is actually an inconsistency pattern in keys/actor.rs:

    • WorkflowIdKey - now has pub actor_id (after this PR)
    • CreateTsKey, RunnerIdKey, ConnectableKey, etc. - have private actor_id fields with no getters
    • HibernatingRequestKey - has pub gateway_id and pub request_id

    The file actually has two patterns:

    • Private fields with no accessors (most common)
    • Public fields (used when external code needs direct access)

Recommendation

The change is functionally correct and improves the code. However, consider one of these approaches for maximum consistency:

Option A (Recommended): Keep the field private and access it directly in the same way as other key structs

  • Change pub actor_id back to actor_id (private)
  • In the workflow code, access it directly through the struct's internal implementation patterns

Option B: Accept this as-is, recognizing that public field access is appropriate when external code needs to read the ID

Given that the workflow code needs to read the actor_id for logic at lines 167 and 171, Option B (accepting this PR as-is) is actually the pragmatic choice. The public field enables clean, direct access for legitimate use cases.

Approval

This PR is approved. The change is correct, improves code clarity, and maintains consistency with the codebase patterns for key structs that need external field access.

Minor suggestion: Fix the typo in the PR title if possible.


Review generated by Claude Code

@MasterPtato MasterPtato force-pushed the 01-06-fix_fix_dashboards_add_pb_metrics_fix_listen_n_order branch from 63497dc to ed662df Compare January 7, 2026 20:29
@MasterPtato MasterPtato force-pushed the 01-06-chore_remove_method_from_key_for_consistently branch from 39a5303 to 33639ab Compare January 7, 2026 20:29
@MasterPtato MasterPtato changed the title chore: remove method from key for consistently chore: remove method from key for consistency Jan 7, 2026
@graphite-app
Copy link
Contributor

graphite-app bot commented Jan 7, 2026

Merge activity

  • Jan 7, 8:57 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Jan 7, 8:57 PM UTC: CI is running for this pull request on a draft pull request (#3779) due to your merge queue CI optimization settings.
  • Jan 7, 8:58 PM UTC: Merged by the Graphite merge queue via draft PR: #3779.

@graphite-app graphite-app bot closed this Jan 7, 2026
@graphite-app graphite-app bot deleted the 01-06-chore_remove_method_from_key_for_consistently branch January 7, 2026 20: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.

3 participants