fix: reject invalid SES regions#424
Conversation
Entire-Checkpoint: 26b0394dad5f
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThe change adds shared SES region validation and normalization. The configured default region is exposed through the admin API and loaded by the SES settings form, which handles loading and retry states and validates regions before quota lookup. Environment configuration, admin procedures, domain creation, available-region output, and SES setting creation use the shared schema. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. Comment |
Deploying usesend with
|
| Latest commit: |
c9432f7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9f12cf78.usesend.pages.dev |
| Branch Preview URL: | https://codex-find-simple-github-iss.usesend.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/src/server/api/routers/domain.ts (1)
25-37: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftAdd tRPC regression coverage for region normalization.
The helper is unit-tested, but the router wiring is not. Add
*.trpc.test.tscases proving blank legacy regions are omitted, normalized duplicates collapse, andcreateDomainrejects blank regions while trimming valid input.As per coding guidelines, API or behavior changes in
apps/webrequire corresponding tests using the*.trpc.test.tsnaming convention.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/server/api/routers/domain.ts` around lines 25 - 37, Add *.trpc.test.ts regression cases for the domain router’s getAvailableRegions and createDomain procedures: verify blank legacy regions are omitted, normalized duplicate regions collapse, and createDomain rejects blank regions while trimming valid input. Exercise the router wiring through tRPC rather than testing only getValidSesRegions directly.Source: Coding guidelines
apps/web/src/server/api/routers/admin.ts (1)
71-99: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftAdd tRPC coverage for the new region contract.
The new default-region query and region input schemas change API behavior. Add
*.trpc.test.tscases covering the default value, trimmed regions, and rejection of empty/whitespace regions for both quota lookup and SES-setting creation.As per coding guidelines, API or behavior changes in
apps/webrequire corresponding tests using the*.trpc.test.tsnaming convention.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/server/api/routers/admin.ts` around lines 71 - 99, Add *.trpc.test.ts coverage for getDefaultSesRegion, getQuotaForRegion, and addSesSettings. Verify the default-region query returns the configured default, valid region inputs are trimmed and accepted, and empty or whitespace-only regions are rejected for both quota lookup and SES-setting creation.Source: Coding guidelines
apps/web/src/server/service/ses-settings-service.ts (1)
55-70: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftAdd a service-level normalization regression test.
Verify that a padded region passed to
createSesSettingis normalized before duplicate checks, persistence, and AWS calls.As per coding guidelines, behavior changes in
apps/webrequire corresponding unit or integration tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/server/service/ses-settings-service.ts` around lines 55 - 70, Add a service-level regression test for createSesSetting that passes a padded region, verifies it is normalized before the duplicate-cache check, and confirms the normalized value is used for persistence and AWS calls. Mock the required initialization and dependencies, and cover the expected normalized-region behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/components/settings/AddSesSettings.tsx`:
- Around line 52-96: Add compatible unit or integration tests for the
AddSesSettings component and SesSettingsForm covering loading, retry after a
missing default region, rendering the fetched default region, normalized valid
input, invalid input, and a rejected quota request. Reuse existing testing
utilities and API-mocking patterns from the surrounding apps/web tests, and
assert the user-visible outcomes for each state.
- Around line 142-148: Update onRegionInputOutOfFocus around
getQuotaForRegion.fetch to catch rejected quota lookups, including unsupported
regions, and prevent unhandled promises. On failure, surface an appropriate
region/form error or apply the existing safe send-rate fallback while preserving
the successful quota behavior.
In `@apps/web/src/server/service/ses-settings-service.ts`:
- Line 66: Update the getSetting read path to normalize the requested SES region
with the shared sesRegionSchema before using it as a cache key or querying
settings, so whitespace is trimmed and empty input follows the existing
default-region behavior. Keep createSesSetting’s validation unchanged and reuse
the normalized region consistently throughout getSetting.
---
Outside diff comments:
In `@apps/web/src/server/api/routers/admin.ts`:
- Around line 71-99: Add *.trpc.test.ts coverage for getDefaultSesRegion,
getQuotaForRegion, and addSesSettings. Verify the default-region query returns
the configured default, valid region inputs are trimmed and accepted, and empty
or whitespace-only regions are rejected for both quota lookup and SES-setting
creation.
In `@apps/web/src/server/api/routers/domain.ts`:
- Around line 25-37: Add *.trpc.test.ts regression cases for the domain router’s
getAvailableRegions and createDomain procedures: verify blank legacy regions are
omitted, normalized duplicate regions collapse, and createDomain rejects blank
regions while trimming valid input. Exercise the router wiring through tRPC
rather than testing only getValidSesRegions directly.
In `@apps/web/src/server/service/ses-settings-service.ts`:
- Around line 55-70: Add a service-level regression test for createSesSetting
that passes a padded region, verifies it is normalized before the
duplicate-cache check, and confirms the normalized value is used for persistence
and AWS calls. Mock the required initialization and dependencies, and cover the
expected normalized-region behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6b83478-4cea-46a6-be68-06c2cbfc3614
📒 Files selected for processing (7)
apps/web/src/components/settings/AddSesSettings.tsxapps/web/src/env.jsapps/web/src/lib/zod/ses-setting-schema.tsapps/web/src/lib/zod/ses-setting-schema.unit.test.tsapps/web/src/server/api/routers/admin.tsapps/web/src/server/api/routers/domain.tsapps/web/src/server/service/ses-settings-service.ts
| const defaultRegion = api.admin.getDefaultSesRegion.useQuery(); | ||
|
|
||
| if (defaultRegion.isLoading) { | ||
| return ( | ||
| <div className="flex min-h-[500px] items-center justify-center"> | ||
| <Spinner className="h-5 w-5" /> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| if (!defaultRegion.data) { | ||
| return ( | ||
| <div className="flex min-h-[500px] flex-col items-center justify-center gap-3 text-center"> | ||
| <p className="text-sm text-muted-foreground" role="alert"> | ||
| Failed to load the default AWS region. | ||
| </p> | ||
| <Button | ||
| type="button" | ||
| variant="outline" | ||
| onClick={() => void defaultRegion.refetch()} | ||
| > | ||
| Retry | ||
| </Button> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <SesSettingsForm | ||
| defaultRegion={defaultRegion.data} | ||
| onSuccess={onSuccess} | ||
| /> | ||
| ); | ||
| }; | ||
|
|
||
| const SesSettingsForm: React.FC< | ||
| SesSettingsProps & { defaultRegion: string } | ||
| > = ({ defaultRegion, onSuccess }) => { | ||
| const addSesSettings = api.admin.addSesSettings.useMutation(); | ||
| const utils = api.useUtils(); | ||
|
|
||
| const form = useForm<z.infer<typeof FormSchema>>({ | ||
| resolver: zodResolver(FormSchema), | ||
| defaultValues: { | ||
| region: "", | ||
| region: defaultRegion, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add tests for the new settings-form states.
Cover loading, retry, the fetched default region, normalized valid input, invalid input, and a rejected quota request.
As per coding guidelines, behavior changes in apps/web require compatible unit or integration tests.
Also applies to: 141-148
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/components/settings/AddSesSettings.tsx` around lines 52 - 96,
Add compatible unit or integration tests for the AddSesSettings component and
SesSettingsForm covering loading, retry after a missing default region,
rendering the fetched default region, normalized valid input, invalid input, and
a rejected quota request. Reuse existing testing utilities and API-mocking
patterns from the surrounding apps/web tests, and assert the user-visible
outcomes for each state.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/server/service/ses-settings-service.ts`:
- Around line 31-36: Update invalidateCache() to normalize persisted region keys
with the same trimming/schema rules used by getSetting() and createSesSetting().
Skip blank or invalid regions while rebuilding this.cache, and define
deterministic handling when multiple raw keys normalize to the same region so
getSetting() and duplicate checks use the normalized entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff29bac3-66c3-4b53-9530-35de9f049c48
📒 Files selected for processing (2)
apps/web/src/components/settings/AddSesSettings.tsxapps/web/src/server/service/ses-settings-service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/components/settings/AddSesSettings.tsx
| const normalizedRegion = sesRegionSchema.parse(region); | ||
|
|
||
| await this.checkInitialized(); | ||
|
|
||
| if (this.cache[region]) { | ||
| return this.cache[region] as SesSetting; | ||
| if (this.cache[normalizedRegion]) { | ||
| return this.cache[normalizedRegion] as SesSetting; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Normalize cached settings before using normalized region keys.
getSetting and createSesSetting now use trimmed regions, but invalidateCache() still stores entries under raw persisted values. A legacy " us-east-1 " row therefore cannot be found by getSetting("us-east-1"), and the duplicate check can permit a second setting for the same normalized region. Normalize valid legacy keys while rebuilding this.cache, skip blank entries, and define collision handling.
Also applies to: 68-72
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/server/service/ses-settings-service.ts` around lines 31 - 36,
Update invalidateCache() to normalize persisted region keys with the same
trimming/schema rules used by getSetting() and createSesSetting(). Skip blank or
invalid regions while rebuilding this.cache, and define deterministic handling
when multiple raw keys normalize to the same region so getSetting() and
duplicate checks use the normalized entry.
1 participant
Closes #85
What changed
AWS_DEFAULT_REGIONbefore mounting the SES settings form, with stable loading and retry statesRoot cause
The SES settings form initialized
regionas an empty string while displayingus-east-1only as a placeholder. Both client and server schemas usedz.string(), which accepts empty and whitespace-only values. Older versions could persist that invalid setting before AWS provisioning failed. When a valid setting was added later, the Add Domain dialog rendered both regions and passed the blank one to Radix as<SelectItem value="">, causing the client-side exception.Impact
New blank SES regions are rejected before AWS side effects. Existing installations with a legacy blank row can open Add Domain safely; only normalized, valid region choices are shown. The invalid row remains visible in Admin for deliberate cleanup.
Verification
pnpm --filter=web exec vitest run -c vitest.unit.config.ts src/lib/zod/ses-setting-schema.unit.test.ts(3 tests passed)apps/web/src/env.jspassed ESLint with--quietSummary by cubic
Reject invalid SES regions and normalize them across the form, API, env, and service to stop blank-region crashes in Add Domain. The default region now loads before the form; region choices are cleaned and deduped so existing installs can open Add Domain safely.
sesRegionSchemato trim/validate regions and reject blanks;AWS_DEFAULT_REGIONis validated inline (trim + required) to keep env loading Node-compatible.AWS_DEFAULT_REGIONbefore rendering the SES settings form with loading/retry states.Written for commit c9432f7. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
AWS_DEFAULT_REGIONvalidation was tightened to reject blank/whitespace-only values.Tests