Fix location detection to only return valid ISO-3166 country codes#231
Merged
Conversation
… unknown
Previously getLocation() fell through to returning the raw timezone when the
zone wasn't mapped to a country, leaking values like "UTC" or "Etc/GMT+7"
into the location field. Clean the COUNTRY_LIST of non-ISO self-mapped
entries (POSIX abbreviations, UTC/GMT aliases, all Etc/*, Factory), collapse
the stale multi-country rows ("US,CA" -> "US", "GU,MP" -> "GU"), add the
legacy Asia/Kostanay alias, and guard getLocation() so only ISO-3166 alpha-2
codes ever escape.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refines the COUNTRY_LIST configuration by removing non-ISO-3166 alpha-2 entries and correcting multi-country mappings. The EventFactory.getLocation method now strictly validates that timezone lookups return a valid two-letter country code, and comprehensive unit tests have been added to ensure correct behavior. Feedback suggests improving type safety during the lookup and optimizing performance by defining the validation regex as a constant.
Comment on lines
+83
to
+88
| const mapped = COUNTRY_LIST[timezone as keyof typeof COUNTRY_LIST]; | ||
| // Only emit ISO-3166 alpha-2. Anything else (including the raw | ||
| // timezone string) is treated as unknown. | ||
| return typeof mapped === "string" && /^[A-Z]{2}$/.test(mapped) | ||
| ? mapped | ||
| : ""; |
There was a problem hiding this comment.
The current implementation has two minor areas for improvement:
- Type Safety: The type assertion
timezone as keyof typeof COUNTRY_LISTis technically incorrect becausetimezoneis a dynamic string that might not be a key in the constant object. This also makes thetypeof mapped === "string"check appear redundant to the TypeScript compiler (which assumes it must be a string based on the assertion). A safer approach is to treat the lookup as a generic record access. - Performance: The regular expression
/^[A-Z]{2}$/is recreated on every call togetLocation(). While modern engines optimize literal regexes, it is better practice to define it as a constant outside the method to avoid redundant instantiation.
Consider refining the lookup and the regex handling as shown in the suggestion.
const mapped = (COUNTRY_LIST as Record<string, string>)[timezone];
// Only emit ISO-3166 alpha-2. Anything else (including the raw
// timezone string) is treated as unknown.
const ISO_3166_ALPHA_2_REGEX = /^[A-Z]{2}$/;
return typeof mapped === "string" && ISO_3166_ALPHA_2_REGEX.test(mapped)
? mapped
: "";
Contributor
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…in getLocation Address review feedback: the `timezone as keyof typeof COUNTRY_LIST` cast narrows to a literal union the runtime string may not belong to, which also made the `typeof mapped === "string"` guard look redundant to the compiler. Cast through `Record<string, string>` instead, and hoist the validator regex to module scope so it isn't re-parsed on every call.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…RY_LIST Moved from the stray position between Asia/Qatar and Asia/Qostanay up to between Asia/Kolkata and Asia/Krasnoyarsk, matching the alphabetical convention used throughout the table.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes the location detection logic in EventFactory to only return valid ISO-3166 alpha-2 country codes, preventing invalid or ambiguous timezone mappings from being exposed.
Key Changes
America/Phoenix: Changed from "US,CA" to "US"Pacific/Guam: Changed from "GU,MP" to "GU"Asia/Kostanay→ "KZ"EventFactoryLocation.spec.tsvalidating:Implementation Details
The location detection now enforces strict validation using a regex pattern
/^[A-Z]{2}$/to ensure only valid ISO-3166 alpha-2 codes are returned. This prevents ambiguous or non-standard timezone identifiers from being included in event context data.https://claude.ai/code/session_01Tz1XFApBYhb1GSppB1PHds