feat(ui): native Reader Search (YPE-5748) - #184
Dustin-Kelley wants to merge 1 commit into
Conversation
b186fb8 to
ab134d6
Compare
cb44449 to
a07af81
Compare
Search belongs in the 5953 spacer. The old row put it between version and gear, and that chrome is gone. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Restacked onto posted by Cursor on behalf of Dustin |
b024ea6 to
c082b45
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return null | ||
| } | ||
| return navigation.pendingRequest | ||
| return navigation.consumePending() |
There was a problem hiding this comment.
When React abandons or suspends a render, consumePending() has already cleared the request, causing the committed reader to miss the requested book, chapter, and version. Please retain the request through rendering and clear it only after the render applying it commits.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/native/bible-reader-navigation.ts
Line: 122
Comment:
**Navigation is consumed before commit**
When React abandons or suspends a render, `consumePending()` has already cleared the request, causing the committed reader to miss the requested book, chapter, and version. Please retain the request through rendering and clear it only after the render applying it commits.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| function ReaderScreen() { | ||
| const navigation = useMemo(() => { | ||
| const readerNavigation = createBibleReaderNavigation() | ||
| readerNavigation.request({ versionId: 111, bookId: 'JHN', chapter: 3, verse: 16 }) | ||
| return readerNavigation | ||
| }, []) | ||
| const navigation = useMemo(() => createBibleReaderNavigation(), []) | ||
|
|
||
| navigation.request({ versionId: 111, bookId: 'JHN', chapter: 3, verse: 16 }) | ||
|
|
||
| return <BibleReader navigation={navigation} defaultVersionId={3034} /> | ||
| } |
There was a problem hiding this comment.
The memoized object is stable, but this unconditional navigation.request() runs after every ReaderScreen render. Consumers copying the example will repeatedly reset the reader to John 3 and notify subscribers on unrelated rerenders.
| function ReaderScreen() { | |
| const navigation = useMemo(() => { | |
| const readerNavigation = createBibleReaderNavigation() | |
| readerNavigation.request({ versionId: 111, bookId: 'JHN', chapter: 3, verse: 16 }) | |
| return readerNavigation | |
| }, []) | |
| const navigation = useMemo(() => createBibleReaderNavigation(), []) | |
| navigation.request({ versionId: 111, bookId: 'JHN', chapter: 3, verse: 16 }) | |
| return <BibleReader navigation={navigation} defaultVersionId={3034} /> | |
| } | |
| function ReaderScreen() { | |
| const navigation = useMemo(() => { | |
| const readerNavigation = createBibleReaderNavigation() | |
| readerNavigation.request({ versionId: 111, bookId: 'JHN', chapter: 3, verse: 16 }) | |
| return readerNavigation | |
| }, []) | |
| return <BibleReader navigation={navigation} defaultVersionId={3034} /> | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: README.md
Line: 167-173
Comment:
**Example requests navigation every render**
The memoized object is stable, but this unconditional `navigation.request()` runs after every `ReaderScreen` render. Consumers copying the example will repeatedly reset the reader to John 3 and notify subscribers on unrelated rerenders.
```suggestion
function ReaderScreen() {
const navigation = useMemo(() => {
const readerNavigation = createBibleReaderNavigation()
readerNavigation.request({ versionId: 111, bookId: 'JHN', chapter: 3, verse: 16 })
return readerNavigation
}, [])
return <BibleReader navigation={navigation} defaultVersionId={3034} />
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| "clearSearch": "Clear search", | ||
| "trendingSearches": "Trending Searches", | ||
| "recentSearches": "Recent Searches", |
There was a problem hiding this comment.
These values are being added directly to generated locale files, so the catalogs diverge from their canonical source and can be overwritten or rejected by localization synchronization. Please add the keys under reactnative.* in platform-localization and let the distribution workflow update both catalogs.
Rule Used: Translation JSON under packages/ui/src/i18n/locales/ (en.json, es.json, fr.json, and any future locale) is generated and synced from the platform-localization repo. Do not hand-edit these files or add/change/remove string values directly in a PR. New... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ui/src/i18n/locales/en.json
Line: 8-10
Comment:
**Generated locale catalogs edited directly**
These values are being added directly to generated locale files, so the catalogs diverge from their canonical source and can be overwritten or rejected by localization synchronization. Please add the keys under `reactnative.*` in platform-localization and let the distribution workflow update both catalogs.
**Rule Used:** Translation JSON under packages/ui/src/i18n/locales/ (en.json, es.json, fr.json, and any future locale) is generated and synced from the platform-localization repo. Do not hand-edit these files or add/change/remove string values directly in a PR. New... ([source](https://github.com/youversion/platform-sdk-reactnative-expo/blob/c082b4527b6a8a3cb8c56a4faad4ad6ef9edf13a/.greptile/config.json))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
Locale JSON guard is red because this PR adds Unit, lint, typecheck, build, and locale parity are green. Do not merge from this comment. posted by Cursor on behalf of Dustin |
1 participant
Summary
showToolbar).Stack
Search(feat(core): wrap platform-core SearchClient (YPE-5746) #180)ype-5745-reader-navigationYPE-5747 (scroll/focus) is out of this stack.
Test plan
showToolbaris true; absent when false; no spare barInputand Search sheet are not public exportspnpm typecheck/pnpm test/pnpm lintfrom worktree rootMade with Cursor
The PR does not yet appear safe to merge because navigation can still be lost before commit, the public example repeatedly resets the reader, and generated localization catalogs remain hand-edited.
Fix with agent prompt
Summary
The PR adds native Reader Search with a toolbar entry point, debounced suggestions, trending and recent searches, paginated verse results, and chapter-only navigation from selected results.
Diagram
Reviews (2) · Last reviewed commit: "feat(ui): restack Reader Search onto Swi..."