E/null dict #675
E/null dict #675
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
5 Skipped Deployments
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR introduces comprehensive support for null dictionary entries throughout the General Translation library's React and Next.js packages. The changes systematically update the type system to allow dictionary entries to be string | null (previously just string), and modify all related functions to handle null values gracefully.
The core change is in types.ts where the Entry type is expanded from string to string | null. This cascades through the entire dictionary handling system, requiring updates to functions like getEntryAndMetadata, collectUntranslatedEntries, injectFallbacks, and translation hooks. The changes enable the system to properly represent and process cases where translations may be explicitly null (indicating missing or intentionally empty content) rather than just undefined.
Key architectural improvements include:
- Type guards (
isDictionaryEntry,isValidDictionaryEntry) now properly validate null entries - Translation functions add null checks to prevent attempting translation on null sources
- Dictionary merging and injection functions handle null entries without errors
- Source maps are enabled in both packages to improve debugging capabilities
- Version bumps to alpha releases (10.6.2-alpha.0 for React, 6.6.2-alpha.0 for Next.js) for testing
The changes maintain backward compatibility while extending the dictionary system's capability to handle incomplete or intentionally empty translation scenarios, which is common in internationalization workflows.
Confidence score: 2/5
- This PR contains several critical bugs that could cause runtime failures, particularly in dictionary traversal and creation logic
- Score reflects multiple type safety issues, logic errors, and inconsistencies that need resolution before merging
- Pay close attention to
packages/react/src/dictionaries/getSubtree.tsandpackages/react/src/dictionaries/indexDict.tswhich contain significant logic flaws
Context used:
Rule - Remove console.log statements and debug logging from production code before merging. (link)
20 files reviewed, 7 comments
| if (typeof value?.[0] === 'object' && value?.[0] !== null) { | ||
| return false; |
There was a problem hiding this comment.
logic: The validation logic has been inverted - now rejects objects (except null) instead of accepting only strings. This creates inconsistency with isDictionaryEntry function which uses different validation rules for the same data structure.
| */ | ||
| export function get(dictionary: Dictionary, id: string | number) { | ||
| if (dictionary == null) { | ||
| if (dictionary === undefined) { |
There was a problem hiding this comment.
logic: This change allows null dictionaries to pass through, but accessing properties on null (line 12: dictionary[id as number] or line 14: dictionary[id as string]) will cause runtime errors. Consider adding explicit null handling.
| if (dictionary === undefined) { | |
| if (dictionary === undefined || dictionary === null) { |
| export function get(dictionary: Dictionary, id: string | number) { | ||
| if (dictionary == null) { | ||
| if (dictionary === undefined) { | ||
| throw new Error('Cannot index into an undefined dictionary'); |
There was a problem hiding this comment.
style: Error message should be updated to reflect that only undefined dictionaries are rejected, or the function should handle null dictionaries explicitly.
|
|
||
| // Don't translate non-string entries | ||
| if (typeof entry !== 'string') { | ||
| injectEntry(entry, dictionaryTranslations!, id, dictionary); |
There was a problem hiding this comment.
logic: This assumes dictionaryTranslations is not null due to the ! operator, but it could be undefined based on line 67-69 logic
| if (typeof entry !== 'string') { | ||
| return renderMessage(entry, [defaultLocale]); | ||
| } |
There was a problem hiding this comment.
style: This type check is redundant since line 59 already handles non-string entries. Consider removing or add a comment explaining why this additional check is necessary.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| // Check if it's a string (Entry) | ||
| if (typeof value === 'string') { | ||
| // Check if it's an entry | ||
| if (value === null || typeof value !== 'object') { |
There was a problem hiding this comment.
Bug: Dictionary Validation Fails for Non-Object Primitives
The validation logic in isValidDictionaryEntry and isDictionaryEntry is too permissive. The condition value === null || typeof value !== 'object' incorrectly validates any non-object primitive (e.g., numbers, booleans, undefined) as a DictionaryEntry. This contradicts the Entry type (string | null), creating a type safety mismatch and potential runtime errors.
Additional Locations (2)
|
|
||
| if (Array.isArray(value)) { | ||
| if (typeof value?.[0] !== 'string') { | ||
| if (typeof value?.[0] === 'object' && value?.[0] !== null) { |
There was a problem hiding this comment.
Bug: Array Validation Fails for Non-String Primitives
The array validation logic in isValidDictionaryEntry and isDictionaryEntry incorrectly allows non-string, non-null primitive types as the first element of a dictionary entry array. The condition typeof value[0] === 'object' && value[0] !== null only rejects non-null objects, contradicting the expected string or null type for the first element.
Additional Locations (1)
|
|
||
| // Merge dictionary with dictionary translations | ||
| dictionary = mergeDictionaries(dictionary, dictionaryTranslations); | ||
| // dictionary = mergeDictionaries(dictionary, dictionaryTranslations); |
There was a problem hiding this comment.
Bug: Translation Merge Code Commented Out
The dictionary merge operation was commented out, disabling the merging of dictionary translations with the base dictionary. This prevents translations from being applied properly and appears to be accidentally committed debugging code.
No description provided.