From 9268c5a225c48ac671393427eed3e1264dc0dc5c Mon Sep 17 00:00:00 2001 From: Ronen Mars Date: Sat, 1 Aug 2026 18:11:01 +0300 Subject: [PATCH] docs: track the scroll-FAB extraction plan and the hardcoded-strings audit [skip-ci] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both existed only as untracked files in the working tree, so neither survived a fresh clone. The scroll-FAB plan covers the extraction that #476 deferred when the top-FAB overlay fix made the third copy converge on the other two, and no commit since has done it. `docs/superpowers/plans/` is where the repo already keeps active plans, so it goes there alongside the other fourteen. The audit inventories user-facing strings in `app/` and `components/` that are not routed through `react-i18next`. The i18n CI job enforces locale key parity rather than coverage, so nothing else in the repo records which strings are still hardcoded. Its line numbers will drift, but the inventory itself is still accurate — spot-checked against `components/ui/FAB.tsx` and `components/ui/AvatarMenu.tsx`. [skip-ci] --- docs/hardcoded-strings-audit.md | 151 ++++++++++++++++++ .../plans/2026-08-01-extract-scroll-fab.md | 111 +++++++++++++ 2 files changed, 262 insertions(+) create mode 100644 docs/hardcoded-strings-audit.md create mode 100644 docs/superpowers/plans/2026-08-01-extract-scroll-fab.md diff --git a/docs/hardcoded-strings-audit.md b/docs/hardcoded-strings-audit.md new file mode 100644 index 00000000..7bf437b7 --- /dev/null +++ b/docs/hardcoded-strings-audit.md @@ -0,0 +1,151 @@ +# Pre-existing Hardcoded UI Strings + +Audit of user-facing English strings not yet routed through i18n (`react-i18next`). +Scope: `app/` and `components/`, excluding already-localized files +(`app/session/[id].tsx`, `components/terminal/TerminalOutput.tsx`) and worktrees/tests. +Read-only research — nothing here has been fixed yet. + +## app/index.tsx +- L340: `accessibilityLabel="Server status"` — header cloud/status button +- L352: `accessibilityLabel="Search"` — header search toggle button + +## app/_layout.tsx +- L344: `accessibilityLabel="Back"` — custom header back button +- L360: `title: 'Browse'` — Stack.Screen title +- L361: `headerBackTitle: 'Cancel'` — header back title, Browse modal +- L366: `title: 'Settings'` — Stack.Screen title +- L370: `title: 'Manage Favorites'` — Stack.Screen title + +## app/browse.tsx +- L200: `Alert.alert('Error', err.message)` — generic browse error +- L414: `placeholder="Folder name"` — new-folder input +- L432-433: `label: 'Claude'`, `label: 'Codex'` — provider filter options + +## app/conversation/[id].tsx +- L373: `Alert.alert('Favorites error', 'Failed to update favorites')` +- L585: `accessibilityLabel="Conversation info"` — header info button +- L798-810: info modal field labels: `'ID'`, `'Title'`, `'Session Name'`, `'Project Path'`, `'Repo URL'`, `'File Path'`, `'Branch'`, `'Account'`, `'Provider'`, `'Model'`, `'Message Count'`, `'Total Tokens'`, `'Last Activity'` + +## components/ui/FAB.tsx +- L40: `accessibilityLabel="New session"` + +## components/ui/AvatarMenu.tsx +- L15: `accessibilityLabel="Settings"` + +## components/shared/InfoModal.tsx +- L67: `accessibilityLabel="Close"` + +## components/shared/SlashCommandArgModal.tsx +- L79, L107: `accessibilityLabel="Cancel"` — dismiss X and bottom Cancel button + +## components/queue/PromptQueueSheet.tsx +- L107: `placeholder="Add a prompt to queue..."` +- L116: `accessibilityLabel="Add prompt to queue"` + +## components/queue/PlanPreviewSheet.tsx +- L103: `placeholder="Edit the prompt before proceeding..."` + +## components/servers/ServersStatusModal.tsx +- L172: `accessibilityLabel="Server options"` + +## components/servers/NoServersWelcome.tsx +- L41: `accessibilityLabel="Add Server"` + +## components/servers/ServerEditModal.tsx +- L78: `Alert.alert('Discard changes?', 'Your unsaved changes will be lost.', [...])` with buttons `'Keep Editing'` / `'Discard'` +- L174: `accessibilityLabel="Scan QR code"` +- L183: `placeholder="Paste your API token here"` + +## components/servers/ServerListCard.tsx +- ~L80: `server.label || 'Server'` — fallback display text +- L89: `accessibilityLabel="View connection error"` +- L98: `accessibilityLabel="Delete server"` +- L106: `accessibilityLabel="Edit server"` +- L114: `accessibilityLabel="Refresh server info"` + +## components/servers/AddServerScreen.tsx +- L54: `title: 'Add Server'` +- L201: `accessibilityLabel="Scan pairing QR"` +- L261: `placeholder="Work Mac, Home Server…"` +- L275: `placeholder="Paste your API token here"` + +## components/servers/FilterSortSheet.tsx +- L36-38: `label: 'Tree'`, `'Hub'`, `'Classic'` — view-mode options +- L42-45: `label: 'Last message'`, `'Project name'`, `'Created date'`, `'Status'` — sort options +- L115-117: `label: 'Running'`, `'Active'`, `'Idle'` — status filter chips +- L289-291: `label: 'All'`, `'Claude'`, `'Codex'` — provider filter chips + +## components/servers/SortSheet.tsx +- L22-25: `label: 'Project name'`, `'Last message'`, `'Created date'`, `'Status'` + +## components/servers/ServerFilterSheet.tsx +- L27-28: `label: 'Running'`, `'Idle'` +- L33-34: `label: 'Last activity'`, `'Started'` + +## components/servers/AddServerActionSheet.tsx +- L23: `label: 'Add to displayed'` +- L28: `label: 'Display only the new server'` +- L33: `label: 'Change nothing'` + +## components/conversation/ConversationHistoryList.tsx +- L225: `accessibilityLabel="Scroll to top"` + +## components/conversation/ConversationList.tsx +- L203: `placeholder="Search conversations…"` +- L244: `accessibilityLabel="Scroll to top"` +- L254: `accessibilityLabel="Scroll to bottom"` + +## components/conversation/ChatComposer.tsx +- L128: `accessibilityLabel="Attach file"` +- L197, L223: `accessibilityLabel="Expand input"` +- L268: `accessibilityLabel="Minimize input"` + +## components/conversation/LiveConversationView.tsx +- L215: `Alert.alert('Not connected', 'Waiting for connection — try again in a moment.')` +- L224: `Alert.alert('Send failed', ...)` + +## components/terminal/TerminalView.tsx +- L54: `Alert.alert('Send failed', ...)` + +## components/sessions/SessionCard.tsx +- L125: `Alert.alert('Session Actions', session.projectName, [...])` +- L128: `{ text: 'Dismiss', style: 'cancel' }` +- ~L145: accessibilityLabel template embeds hardcoded word "status" + +## components/sessions/hub/SessionRow.tsx +- L45: `options: ['Cancel Session', 'Cancel']` (ActionSheetIOS) +- L49: `Alert.alert('Cancel Session', 'Are you sure?', [...])` +- L52: `{ text: 'No', style: 'cancel' }` +- L55: `text: 'Yes'` +- L64: `Alert.alert('Session Actions', session.projectName, [...])` +- L65: `{ text: 'Cancel Session', style: 'destructive' }` +- L66: `{ text: 'Dismiss', style: 'cancel' }` +- L74: `session.branch || 'no git'` +- ~L77: manual `` `${count} prompt${count===1?'':'s'}` `` pluralization + +## components/tour/TourOverlay.tsx +- L86: `Skip tour` + +## components/tour/FirstShowBanner.tsx +- L35: `Got it` + +## components/onboarding/components/TokenTooltip.tsx +- L30: `Got it` + +## components/onboarding/components/InfoTooltip.tsx +- L38: `Got it` + +## components/onboarding/steps/NotificationsStep.tsx +- L73: `Wake me only when it counts.` — onboarding headline +- L90: `THREADBASE` — likely intentional brand name in preview mockup, flag only, probably exclude + +## Already checked, clean +`app/settings.tsx`, `app/paired-devices.tsx`, `app/session/new.tsx`, +`components/RootErrorBoundary.tsx`, `components/help-feedback.tsx`, `app/backup-restore.tsx`. + +## Notes +- Not exhaustive — broad grep-based sweep, prioritized by directory. A full pass + would also need `app/settings/*`, `app/onboarding/*` steps beyond Notifications, + and remaining `components/**` not listed above. +- `THREADBASE` brand name and similar intentional non-translatable strings should + be excluded from any follow-up localization pass. diff --git a/docs/superpowers/plans/2026-08-01-extract-scroll-fab.md b/docs/superpowers/plans/2026-08-01-extract-scroll-fab.md new file mode 100644 index 00000000..545652c0 --- /dev/null +++ b/docs/superpowers/plans/2026-08-01-extract-scroll-fab.md @@ -0,0 +1,111 @@ +# Extract the three near-identical scroll-to-edge FABs into one component + +Repo `threadbase-mobile`. Branch from `land/integration-prep` after pulling from origin, which now contains `ad16c92e` — the top-FAB overlay fix (#476) that made the third copy converge on the other two and explicitly deferred this extraction. + +## Background + +Three components each hand-roll the same affordance: a small pill pinned to the top or bottom edge of a scrolling list that fades in when you have scrolled away from that edge, and scrolls you back when tapped. + +- `components/conversation/ConversationHistoryList.tsx` — top pill only (its bottom control is a different shape; see below) +- `components/conversation/ConversationList.tsx` — top and bottom pills +- `components/terminal/TerminalOutput.tsx` — top and bottom pills + +Before #476 the first one was visibly wrong: an opaque accent-blue pill that occluded message text. #476 brought it in line with the other two. That is what makes the duplication newly obvious — and it is why this ticket exists rather than being folded into that PR. + +## Read this before deciding anything + +**This may not be worth doing.** The three copies look alike but are not interchangeable, and a careless extraction silently changes behavior on two of three screens. Read all three implementations end to end first, then decide. Reporting back "not worth it, here is why" is an acceptable and possibly correct outcome — say so early rather than forcing a shared component into existence. + +The parts that genuinely differ: + +### 1. The visibility rules contradict each other + +| | top pill shows when | bottom pill shows when | +|---|---|---| +| `ConversationHistoryList` | `y > 200` (direction-agnostic) | `distFromBottom > 100` | +| `ConversationList` | `!scrollingUp && y > 120` (scrolling **down**) | `scrollingUp && distFromBottom > 120` | +| `TerminalOutput` | `scrollingUp && y > 100` (scrolling **up**) | `distFromBottom >= 50` | + +`ConversationList` reveals its top pill while you scroll **down**; `TerminalOutput` reveals its top pill while you scroll **up**. Those are opposite. Any shared component that owns the visibility rule has to keep all three as configuration, which means the "shared" part is a styled shell and a scroll callback — not the logic. + +Do not unify these thresholds or directions. If you think one of them is a bug, that is a separate ticket with its own evidence; do not fix it under cover of a refactor. + +### 2. The scroll plumbing differs for a documented reason + +`ConversationList` drives an `Animated.FlatList` with `useAnimatedScrollHandler` and writes shared values from the worklet. + +`TerminalOutput` and `ConversationHistoryList` drive FlashList v2 and deliberately do **not** use `useAnimatedScrollHandler`. There is a comment at `TerminalOutput.tsx:126-130` recording why: FlashList v2 calls `onScroll` via an `Animated.event` listener, and the worklet wrapper raises `undefined is not a function` inside `RecyclerView`. Preserve that constraint. A shared component that assumes a worklet scroll handler will break both FlashList call sites at runtime, and it will break them in a way unit tests do not catch. + +Note that the two FlashList copies solve it differently — `TerminalOutput` keeps `useState` and mirrors it into a shared value during render (`TerminalOutput.tsx:136-137`), while `ConversationHistoryList` writes the shared value directly from a plain JS `useCallback`. The second is cheaper (no re-render per scroll event) and is the newer of the two. If you converge them, converge on the direct-write approach and say so — but that is a behavior-adjacent change, so verify it on device, not just in Jest. + +### 3. The styling is close but not identical + +| | background | border | text | +|---|---|---|---| +| `ConversationHistoryList`, `ConversationList` | `rgba(31, 111, 235, 0.14)` | `rgba(88, 166, 255, 0.2)` | `rgba(230, 237, 243, 0.6)` | +| `TerminalOutput` | `rgba(31, 111, 235, 0.18)` | `rgba(88, 166, 255, 0.25)` | `rgba(255, 255, 255, 0.7)` | + +`TerminalOutput` sits on a fixed dark `#0d1117` terminal surface, so it is slightly more opaque and its text is pure white — it is not theme-aware, and it should not become theme-aware here. It also carries `minHeight: MIN_TOUCH_TARGET` + `justifyContent: 'center'` that the other two lack; that came from the a11y touch-target work in #405 and must survive. The fade duration is 200ms in `TerminalOutput` and 220ms in the other two. + +Decide deliberately whether the shared component takes a variant prop or whether the terminal keeps its own styling, and justify it in the PR. + +### 4. One "sibling" is not a sibling + +`ConversationHistoryList`'s **bottom** control is not a pill at all. It is a 40×40 round `CaretDown` icon button in the bottom-right gutter, opaque `theme.text.accent`, with a shadow, still conditionally mounted via `useState`. It was deliberately left alone by #476 because it sits in the gutter and does not occlude text. + +Do not sweep it into the extraction to make the numbers look symmetrical. Either leave it exactly as-is, or treat converting it as an explicit, separately-justified decision — it is a different control with a different visual language and a different reason to exist. + +## What a good outcome looks like + +Whatever shape you land on, these must hold: + +- No user-visible behavior change on any of the three screens: same reveal thresholds, same reveal directions, same fade durations, same styling per surface. +- The FlashList worklet constraint is preserved and the comment explaining it survives in a findable place. +- The `MIN_TOUCH_TARGET` floor on the terminal pills survives. +- The net line count goes **down**. If the shared component plus three call sites is longer than the three copies, the abstraction is not paying for itself — abandon it and report that. +- No new props exist "for future flexibility". Every prop must be used by at least two of the three call sites; a prop used by exactly one is a sign the thing should not have been shared. + +## Hard constraints + +**Do not touch the FlashList tuning in `ConversationHistoryList`.** `drawDistance`, `maintainVisibleContentPosition`, `getItemType`, and `contentContainerStyle` were set by #470 after measuring content-height swings up to ±19,898pt causing viewport teleports. Read `git show 48cf2943 -- components/conversation/ConversationHistoryList.tsx` before you start. The FABs are overlays that live *next to* the list — the extraction must not reach into list props. + +**Keep `testID="conversation-scroll-top"`.** `e2e/07_conversation_scroll_gaps.yaml` asserts and taps it. If the extraction changes which element carries the testID, the e2e flow must still pass unmodified — the point of that assertion is that a restyle cannot silently break the control. + +**Do not change the i18n keys.** `nav.scrollToTop` / `nav.scrollToBottom` in `common.json` (en, he, ru, ar) and `action.scrollToBottom` in `conversation.json` are all live. `conversation.json`'s key is used only by the round `CaretDown` button; if you touch that button you risk orphaning the key, and the i18n CI job fails on dead keys. + +## Verification + +1. **Jest.** `npx jest --ci --runInBand --testPathPattern "onversation|erminal" --forceExit` — currently 26 suites / 277 tests green. `TerminalOutput.test.tsx` asserts both pills by accessibility label; those assertions must keep passing untouched. +2. **e2e.** `e2e/07_conversation_scroll_gaps.yaml` must pass **without edits**. Mock server on port 7071 (`node e2e/mock-server.js`), Release build required (`node e2e/ensure-release-build.js`). +3. **Visual.** Screenshot all three surfaces before and after at a scroll offset where the pill is visible, and confirm they are pixel-comparable. A refactor that shifts a pill by 4pt is a regression. +4. `npm run lint`, `npm run typecheck`, `npm run test:i18n`. + +Known pre-existing conditions on this base, none of them yours: `npx tsc --noEmit` reports 14 errors, `npm run lint` reports 5 warnings, and `npm run lint:i18n` fails its `--max-warnings=0` gate on `app/session/[id].tsx`. Confirm each against the base branch with your changes stashed before attributing any of them to your work. + +Gotchas that cost real time recently: + +- Export `SENTRY_DISABLE_AUTO_UPLOAD=true` or the Release build fails at symbol upload with xcodebuild error 65. +- `e2e/ensure-release-build.js` reuses **any** `Threadbase-*` Release build it finds in DerivedData, including one built from a different worktree. It will silently test stale code. Build explicitly with `npx expo run:ios --configuration Release --device ` and confirm the installed `main.jsbundle` contains a string you just added before trusting any result. +- `npx expo run:ios` hangs waiting on an interactive device prompt when several simulators exist; always pass `--device `. +- Maestro 2.6.1's `hideKeyboard` does not dismiss the iOS 26 keyboard; `07_conversation_scroll_gaps.yaml` pairs inline instead of via `setup.yaml`. + +## Workflow + +Work in an isolated worktree: + +```bash +git fetch origin land/integration-prep +git worktree add .worktrees/extract-scroll-fab -b refactor/extract-scroll-fab origin/land/integration-prep +cd .worktrees/extract-scroll-fab && npm ci && (cd ios && bundle exec pod install) +``` + +`bundle exec pod install` in a fresh worktree rewrites three path-dependent checksums in `ios/Podfile.lock` (`ExpoModulesCore`, `ExpoWidgets`, `hermes-engine`). That is environmental drift documented in `CLAUDE.md`; run `scripts/reset-podfile-lock-path-noise.sh` or `git checkout -- ios/Podfile.lock` and keep it out of your commit. + +Open the PR against `land/integration-prep` and stop there — do not merge without being asked. + +## Deliverables + +1. Either the extraction, or a written recommendation not to do it with the specific evidence that led there +2. If extracted: proof of no behavior change on all three surfaces, including before/after screenshots +3. `07_conversation_scroll_gaps.yaml` passing unmodified +4. A net reduction in lines, stated explicitly in the PR