Skip to content
This repository was archived by the owner on Aug 12, 2026. It is now read-only.

Validate and centralize XMTP environment handling - #187

Open
neekolas wants to merge 2 commits into
otr-devfrom
typed-xmtp-env
Open

Validate and centralize XMTP environment handling#187
neekolas wants to merge 2 commits into
otr-devfrom
typed-xmtp-env

Conversation

@neekolas

@neekolas neekolas commented Apr 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

Centralizes XMTP environment handling before adding testnet-specific behavior.

  • Adds typed XMTP_ENV parsing with validation for production, testnet, dev, and local.
  • Adds helpers for production checks and dev-like behavior.
  • Updates existing production/dev behavior branches to use the shared helpers.
  • Keeps local valid and dev-like for tests and local development.

Verification

  • bun check
  • bun test tests/config-environment.test.ts

Note

Centralize and validate XMTP environment handling in config

  • Adds parseXmtpEnv() in config.ts that validates XMTP_ENV against an allowed list (production, testnet, dev, local), throwing at startup if an unsupported value is set.
  • Adds isXmtpProduction() and shouldUseDevBehavior() helpers, replacing direct process.env.XMTP_ENV string comparisons in webhook.ts and v2/index.ts.
  • Adds tests in config-environment.test.ts covering production detection, dev-behavior flags, and invalid value rejection.
  • Risk: services with a non-standard XMTP_ENV value that previously started successfully will now throw on startup.
📊 Macroscope summarized e2145e4. 5 files reviewed, 1 issue evaluated, 0 issues filtered, 1 comment posted

🗂️ Filtered Issues

@coderabbitai

coderabbitai Bot commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

Warning

Rate limit exceeded

@neekolas has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 46 minutes and 19 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 46 minutes and 19 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 979713e8-1bdb-49c7-ba2e-0f231ddcfd96

📥 Commits

Reviewing files that changed from the base of the PR and between e1aa094 and e2145e4.

📒 Files selected for processing (4)
  • src/api/v2/index.ts
  • src/api/v2/notifications/handlers/webhook.ts
  • src/config.ts
  • tests/config-environment.test.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch typed-xmtp-env

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude

claude Bot commented Apr 15, 2026

Copy link
Copy Markdown

Claude finished @neekolas's task —— View job


Code Review Complete ✅

Tasks

  • Read repository guidelines (CLAUDE.md)
  • Review changed files
  • Analyze code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security concerns
  • Review test coverage
  • Provide comprehensive feedback

Summary

This PR successfully centralizes XMTP environment handling with strong type safety and validation. The implementation is well-structured and addresses a clear need to prevent misconfiguration. Overall, the changes are high quality with only minor suggestions for improvement.


✅ Strengths

  1. Strong Type Safety: The use of const assertion and type guards (isValidXmtpEnv) ensures compile-time safety while preventing runtime errors from invalid values.

  2. Fail-Fast Validation: Throwing an error at module load time (line 52 in src/config.ts:52) is the right approach—it's better to crash on startup than silently accept invalid configuration.

  3. Clear Semantic Helpers: isXmtpProduction() and shouldUseDevBehavior() make the intent explicit at call sites, improving code readability compared to raw string comparisons.

  4. Good Test Coverage: The test file covers all critical cases including the validation error case.

  5. Consistent Migration: Both usage sites (src/api/v2/index.ts:42 and src/api/v2/notifications/handlers/webhook.ts:267) were properly updated to use the new helpers.


🔍 Code Quality & Best Practices

Good:

  • Separation of concerns: validation logic is centralized
  • Immutable configuration values (exported as constants)
  • Descriptive error messages with actionable guidance

Suggestion:
Consider exporting isValidXmtpEnv as a public function. While it's currently internal, it could be useful for runtime validation in other contexts (e.g., API endpoints that accept environment parameters).


🐛 Potential Issues

Minor: Default Value Behavior

In src/config.ts:50, the default value "dev" is applied when process.env.XMTP_ENV is undefined or empty string:

export function parseXmtpEnv(value = process.env.XMTP_ENV || "dev"): XmtpEnv {

Observation: This means two distinct scenarios are treated identically:

  • XMTP_ENV not set → defaults to "dev"
  • XMTP_ENV="" (empty string) → defaults to "dev" ⚠️

The latter case could indicate a configuration error (someone explicitly set it to empty). Consider whether you want to distinguish these cases or if the current behavior is intentional.

Impact: Low. In practice, this is unlikely to cause issues, but worth documenting.


⚡ Performance Considerations

No concerns. The changes improve performance by:

  • Caching the parsed/validated value at module load time (eliminating repeated process.env lookups)
  • Using simple boolean checks in helper functions

The one-time startup validation cost is negligible.


🔒 Security Concerns

No issues identified.

The validation logic prevents injection of unexpected values that could lead to unintended behavior. The fail-fast approach ensures misconfigurations are caught before any sensitive operations occur.

One observation: The distinction between NODE_ENV and XMTP_ENV is now clearer, which reduces the risk of accidentally using the wrong environment check.


🧪 Test Coverage

Good coverage with one gap:

The tests in tests/config-environment.test.ts cover:

  • ✅ Production detection (isXmtpProduction)
  • ✅ Dev-like behavior detection (shouldUseDevBehavior)
  • ✅ Invalid value rejection (parseXmtpEnv)

Missing:

  • Default value behavior: A test confirming that parseXmtpEnv() (no arguments) returns "dev" when XMTP_ENV is not set
  • Edge case: How does parseXmtpEnv("") behave? (Related to the observation above)

Suggestion: Add a test case:

test("defaults to dev when XMTP_ENV is not set", () => {
  // This would require mocking/clearing process.env.XMTP_ENV
  expect(parseXmtpEnv(undefined)).toBe("dev");
});

📋 Additional Observations

  1. Breaking Change Documentation: The PR description clearly documents the risk that processes with unrecognized XMTP_ENV will now throw at startup. This is good communication. ✅

  2. Consistency: The helper functions accept an optional xmtpEnv parameter (defaulting to XMTP_ENV), which is useful for testing but may not be needed in production code. This is fine—it adds flexibility without cost.

  3. Migration Path: All existing usages were updated in this PR. A quick check confirms no lingering process.env.XMTP_ENV references in the changed files. ✅


🎯 Recommendations

Priority: Low (Nice-to-have improvements)

  1. Add test for default behavior (see Test Coverage section)
  2. Consider documenting the empty string behavior in a code comment
  3. Optional: Export isValidXmtpEnv if you anticipate needing runtime validation elsewhere

Verdict

Approve ✅ — This is a solid refactoring that improves type safety and code clarity. The minor suggestions above are enhancements, not blockers.


neekolas commented Apr 15, 2026

Copy link
Copy Markdown
Collaborator Author

@neekolas
neekolas force-pushed the ci-test-format-baseline branch from 4d94e23 to e1aa094 Compare April 15, 2026 22:38
@neekolas
neekolas marked this pull request as ready for review April 15, 2026 23:14
@neekolas
neekolas requested a review from a team as a code owner April 15, 2026 23:14
@macroscopeapp

macroscopeapp Bot commented Apr 15, 2026

Copy link
Copy Markdown

Approvability

Verdict: Needs human review

This refactor centralizes XMTP environment validation with equivalent behavioral logic, but all 8 modified files are owned by @xmtplabs/engineering and the author is not a designated code owner for any of them.

You can customize Macroscope's approvability policy. Learn more.

Base automatically changed from ci-test-format-baseline to otr-dev May 6, 2026 10:17
> **Status**: Draft
> **Branch**: `feat/invite-code-multi-use`
> **Parent PR**: #183 (Invite code gating for Instant Assistant)
> **Branch**: `feat/invite-code-multi-use` > **Parent PR**: #183 (Invite code gating for Instant Assistant)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Low plans/invite-code-multi-use.md:4

The > **Branch**: and > **Parent PR**: metadata lines were collapsed onto a single line, so the literal > character appears mid-line and > **Parent PR**: is no longer rendered as a separate blockquote line. This malformats the document header.

Suggested change
> **Branch**: `feat/invite-code-multi-use` > **Parent PR**: #183 (Invite code gating for Instant Assistant)
> **Branch**: `feat/invite-code-multi-use`
> **Parent PR**: #183 (Invite code gating for Instant Assistant)
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file docs/plans/invite-code-multi-use.md around line 4:

The `> **Branch**:` and `> **Parent PR**:` metadata lines were collapsed onto a single line, so the literal `>` character appears mid-line and `> **Parent PR**:` is no longer rendered as a separate blockquote line. This malformats the document header.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants