Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Nov 21, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Error Error Nov 21, 2025 4:58am
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 21, 2025 4:58am
rivet-inspector Ignored Ignored Preview Nov 21, 2025 4:58am
rivet-site Ignored Ignored Preview Nov 21, 2025 4:58am

Copy link
Member Author

NathanFlurry commented Nov 21, 2025

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 Nov 21, 2025

Code Review

Thanks for this PR! I've reviewed the changes and have some feedback:

Issues Found

1. Typo in PR Title

The PR title has a typo: "speciying" should be "specifying"

2. Breaking API Change ⚠️

The change to getStoragePath() removes the customPath parameter, which is a breaking change for any code that was calling getStoragePath(customPath). While I see it's only exported and used in one place within the codebase, this could affect external consumers.

Before:

export function getStoragePath(customPath?: string): string {
  const dataPath = getDataPath("rivetkit");
  const pathToHash = customPath || process.cwd();
  const dirHash = createHashForPath(pathToHash);
  const path = getNodePath();
  return path.join(dataPath, dirHash);
}

After:

export function getStoragePath(): string {
  const dataPath = getDataPath("rivetkit");
  const dirHash = createHashForPath(process.cwd());
  const path = getNodePath();
  return path.join(dataPath, dirHash);
}

3. Logic Change in FileSystemGlobalState Constructor 🔍

The logic has subtly changed:

Before:

this.#storagePath = persist ? getStoragePath(customPath) : "/tmp";

After:

this.#storagePath = persist ? (customPath ?? getStoragePath()) : "/tmp";

Previous behavior: When customPath was provided, it would be hashed and combined with the data directory path (e.g., ~/.local/share/rivetkit/custom-path-abc123)

New behavior: When customPath is provided, it's used directly as-is (e.g., /my/custom/path)

This is actually a significant behavioral change! The old implementation would:

  • Take customPath → hash it → create a subdirectory under the platform data directory
  • The new implementation uses customPath as an absolute path directly

Questions

  1. Was this behavioral change intentional? The old behavior ensured all RivetKit data lived under a platform-appropriate data directory. The new behavior allows completely custom paths outside the standard locations.

  2. Should there be validation that customPath is an absolute path if provided? Currently, a relative path could cause issues.

  3. Documentation: Should this parameter be documented in JSDoc comments to clarify the expected path format?

Recommendations

  1. Fix the typo in the PR title

  2. Consider adding validation for customPath (e.g., ensure it's absolute)

  3. Add JSDoc documentation to clarify the customPath parameter behavior:

    /**
     * @param persist - Whether to persist actor state to disk
     * @param customPath - Optional absolute path for storage. If not provided, uses platform-specific data directory
     */
    constructor(persist: boolean = true, customPath?: string)
  4. Consider adding a test case that specifically tests the customPath functionality in the test file

Positive Notes ✅

  • The re-export of createFileSystemDriver and createMemoryDriver in mod.ts is good
  • The test file already uses a custom path pattern (/tmp/test-UUID), which validates the new behavior works
  • The implementation is clean and straightforward

Let me know if you'd like me to clarify any of these points or if the behavioral change was intentional!

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 21, 2025

Merge activity

  • Nov 21, 5:25 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
  • Nov 21, 5:25 AM UTC: CI is running for this pull request on a draft pull request (#3520) due to your merge queue CI optimization settings.
  • Nov 21, 5:26 AM UTC: Merged by the Graphite merge queue via draft PR: #3520.

@graphite-app graphite-app bot closed this Nov 21, 2025
@graphite-app graphite-app bot deleted the 11-20-chore_rivetkit_allow_speciying_custom_path_for_createfilesystemdriver branch November 21, 2025 05:26
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.

2 participants