YPE-1565: Show error when YouVersionProvider has no appKey#264
YPE-1565: Show error when YouVersionProvider has no appKey#264cameronapak wants to merge 7 commits into
Conversation
Render a styled "Missing app key" message instead of a blank page; hooks provider throws for hooks-only consumers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 8773650 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
role="alert" already implies aria-live="assertive"; the explicit polite value downgraded announcement urgency for a config error. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Hey @camrun91, what do we do about locales that we add that aren't yet Crowdin translated? Do we put in potential strings or leave those in English in these locale files?
There was a problem hiding this comment.
What do you mean new locals? If you are talking adding new strings the process will be to add them to the file in the localization repo. This is a good test to find out the best way to do this as the english source files shouldn't need to be translated and we need a process to make this quicker
There was a problem hiding this comment.
Okay, so does the localization repo need a PR and that PR merged in before I can work on this? @camrun91
There was a problem hiding this comment.
I think this got handled correct?
- Remove redundant `missingAppKey*` translation keys - Reuse generic `errorHeading` and `invalidAppKeyError` strings - Add a `console.error` in `YouVersionProvider` for developers
The previous commit reused errorHeading/invalidAppKeyError and moved the actionable developer guidance to console.error. Update the assertions to match: expect "Error" instead of "Missing app key", and verify the console.error guidance fires. Also refresh the guard comment to reflect that the panel is intentionally generic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@Dustin-Kelley can you review this? I'm no longer adding new strings to this and re-using old ones P.S. I'll learn the new strings process and share that with you soon |
| if (!appKey?.trim()) { | ||
| throw new Error( | ||
| 'YouVersionProvider: a non-empty "appKey" is required. If you load it from an ' + | ||
| 'environment variable, make sure it is set and restart your dev server.', | ||
| ); | ||
| } | ||
|
|
||
| const resolvedTheme = useResolvedTheme(theme); |
There was a problem hiding this comment.
The throw guard on line 87 exits the function before useResolvedTheme, useMemo, and useEffect are called (lines 94–116). React requires hooks to be called in the same order on every render. If a component ever receives a valid appKey (hooks run), then is re-rendered with an empty/undefined one (hooks are skipped because the throw fires first), React will detect an inconsistent hook count and either log a rules-of-hooks error or behave unpredictably. The react-hooks/rules-of-hooks ESLint rule flags exactly this pattern.
The guard should be moved to run after all hook invocations, or the component should be split so the hook-bearing path is never entered when appKey is invalid.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/hooks/src/context/YouVersionProvider.tsx
Line: 87-94
Comment:
**Conditional throw placed before hook calls violates Rules of Hooks**
The `throw` guard on line 87 exits the function before `useResolvedTheme`, `useMemo`, and `useEffect` are called (lines 94–116). React requires hooks to be called in the same order on every render. If a component ever receives a valid `appKey` (hooks run), then is re-rendered with an empty/undefined one (hooks are skipped because the throw fires first), React will detect an inconsistent hook count and either log a rules-of-hooks error or behave unpredictably. The `react-hooks/rules-of-hooks` ESLint rule flags exactly this pattern.
The guard should be moved to run after all hook invocations, or the component should be split so the hook-bearing path is never entered when `appKey` is invalid.
How can I resolve this? If you propose a fix, please make it concise.
2 participants
Empty/missing
appKeyrendered a blank page. Now: UI provider shows a styled "Missing app key" message; hooks provider throws.MissingAppKeypanel + Storybook storyYPE-1565
🤖 Generated with Claude Code
Greptile Summary
This PR surfaces a clear, styled error when
YouVersionProviderreceives a missing or emptyappKey, replacing the previous blank-page behaviour. The UI package renders aMissingAppKeypanel and the hooks package throws a descriptive error.YouVersionProviderrenders a styledMissingAppKeypanel with i18n text and writes a developer-readable message toconsole.error; base provider is never invoked with an invalid key.appKeyis missing, giving hooks-only consumers a clear error (note: this placement raises a Rules-of-Hooks concern already flagged in a previous review thread).Confidence Score: 5/5
Safe to merge once the Rules-of-Hooks concern in the hooks provider (raised in the previous review thread) is resolved; the UI-package changes are well-structured and tested.
The UI and hooks providers each add a focused, well-tested guard for an invalid appKey. Tests cover undefined, empty-string, and whitespace inputs for both packages. The only open concern — the throw placed before hook calls in the hooks provider — was already flagged in a previous review thread and is not a new finding in this pass. The remaining observations here are non-blocking style notes.
packages/hooks/src/context/YouVersionProvider.tsx — the throw-before-hooks placement warrants a follow-up; packages/ui/src/components/YouVersionProvider.tsx — console.error in the render body.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[YouVersionProvider receives props] --> B{appKey missing or empty?} B -- Yes, UI package --> C[console.error developer message] C --> D[Render YvStyles + MissingAppKey panel] D --> E[resolveTheme static snapshot for panel theme] B -- Yes, hooks package --> F[throw Error] F --> G[Nearest error boundary catches] B -- No, both packages --> H[UI: pass through to BaseYouVersionProvider] H --> I[Hooks: call useResolvedTheme, useMemo, useEffect] I --> J[Render children with context]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A[YouVersionProvider receives props] --> B{appKey missing or empty?} B -- Yes, UI package --> C[console.error developer message] C --> D[Render YvStyles + MissingAppKey panel] D --> E[resolveTheme static snapshot for panel theme] B -- Yes, hooks package --> F[throw Error] F --> G[Nearest error boundary catches] B -- No, both packages --> H[UI: pass through to BaseYouVersionProvider] H --> I[Hooks: call useResolvedTheme, useMemo, useEffect] I --> J[Render children with context]Reviews (6): Last reviewed commit: "Merge branch 'main' into YPE-1565-missin..." | Re-trigger Greptile