feat(mobile): review proposed plans from the phone - #148
Merged
Conversation
The fork had no mobile artifact. app.config.ts and eas.json are branded end to
end for upstream's Expo project — owner, EAS project id, Apple team,
com.t3tools.t3code bundle ids and an OTA update URL — so there was nothing to
install that could pair with a bkt3 server, even though the protocol side of
apps/mobile has been forked for a while.
Adds a fork build identity in a new app.config.bk.ts (BK T3 Code,
work.beknown.bkt3code.mobile, t3code-bk://, expo-updates disabled) applied
through a two-line seam in app.config.ts, an Android release-signing config
plugin, a build script for both platforms, and a push-triggered workflow that
publishes the APK and an unsigned IPA as GitHub release assets.
Android sideloads the signed APK; iOS ships unsigned for SideStore to re-sign
with a free Apple ID, which is why the build rides upstream's existing
T3CODE_IOS_PERSONAL_TEAM path — that already strips the entitlements a free
Apple ID cannot sign. Builds report ${version}+bk.${sha7} as client_version so a
binary older than the server it paired with is identifiable: T3 has no protocol
handshake, and a stale client silently stops receiving orchestration updates.
Pairing needed no changes; the web QR is a plain https pairing URL the release
build's scanner already accepts.
Claude Opus 5 in T3 Code (Claude Code harness).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…mobile The fork's experimental control-center sidebar keeps all of its pure logic under apps/web, which the React Native app cannot import. Porting the phase-grouped session list to mobile therefore had to either duplicate ~1900 lines or move them, and duplication of the cycle guard and the settle rules would drift. Moves the logic into three fork-owned client-runtime subpaths — state/phaseSidebar, state/phaseSidebarTree, state/provider-rate-limits — and folds in the sibling modules the mobile port also needs: the lifecycle counters, the running-session emphasis decision, the move-under-session candidates, and the unread predicate. Every apps/web file left behind is a re-export shim, so PhaseGroupedSidebar.tsx and its 150-odd existing tests are untouched and still pass. What stayed in apps/web: the helpers that emit Tailwind class names. Tailwind finds literal class strings by scanning source under apps/web, so moving those would have silently dropped the styles from the build. Two things the move had to fix. Every .toSorted() call is now sort-a-copy — Hermes ships no ES2023 change-array-by-copy methods, so those would have worked in every browser test and crashed on a phone; a new test asserts this by deleting the methods from Array.prototype. And resolveSettledTimestamp is copied rather than imported, because its home is an upstream-owned file and re-exporting from there would put a fork edit inside it for no gain. No behaviour change. Claude Opus 5 in T3 Code (Claude Code harness). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Mobile had no way to act on a proposed plan at all: the thread showed a "Plan Ready" pill and the only way forward was a desktop. This adds a native plan-review surface — read the plan, question specific lines, then approve or request changes. Web reviews plans in a Plate rich-text editor, which does not port to a phone. The useful phone operations are read, question, and decide, so this surface is read-plus-annotate and reviewer edits to the plan body stay a desktop affordance; `editedMarkdown` is always null on submit, which the contract already allows. Line selection deliberately copies the diff reviewer's gesture (tap to anchor, tap to extend, tap again to clear) so reviewers moving between the two surfaces do not learn two interactions. Anchoring is by quoted text, re-located against whichever version is on screen, so a quote the agent has since rewritten is still listed rather than silently dropped. Fork-cost notes: the two upstream files touched take one marked line each (an import plus one element in ThreadRouteScreen, a route pair in Stack.tsx), and the banner resolves its own visibility so no conditional leaks into the thread screen. planReviewMarkdown moved from apps/web into client-runtime rather than being copied, since it was already DOM-free and both clients need it. Verified: 31 new tests across the three pure modules, mobile tsc and lint clean, web and client-runtime tsc clean after the move. Model: Claude Opus 5 (1M context), harness: Claude Code in T3 Code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…marked Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reading the whole server-config map re-rendered the banner whenever any unrelated environment's config changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Resolves one conflict in apps/web/src/components/sidebar/PhaseGroupedSidebar.logic.ts. This is the relocation hazard AGENTS.md warns about, not an ordinary content conflict. a768835 moved the phase-sidebar logic out of this file into @t3tools/client-runtime/state/phase-sidebar (leaving only the Tailwind class-name helpers plus an `export *` re-export), while 317f7ce on expbkmain added to the same block in place. Taking either side alone loses work: ours drops the Mattermost feature, theirs re-duplicates logic that now lives in client-runtime. Resolution: keep the relocation, and port expbkmain's two additions into the module the logic moved to — - `PhaseSidebarMattermostLink` + `resolvePhaseSidebarMattermostLink` - `mattermostLinkSupported` on `PhaseSidebarRow` Both were verified to be the *only* things expbkmain added to the block, by diffing its export set against the relocated copy and reading every hunk since the merge base. apps/web needs no change: the existing `export *` re-export means PhaseGroupedSidebar.tsx imports both symbols from exactly the same path as before. Verified: MattermostThreadBadge.test.tsx (6 tests) imports the resolver through the web re-export path and passes, phaseSidebar and planReviewMarkdown suites pass, and web + client-runtime tsc are clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: unavailable · PR result: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
CI caught two failures that the local scoped runs could not: the mobile
suite as a whole, and repo-wide formatting.
`src/lib/connection.test.ts` failed to load at all with "__DEV__ is not
defined". bkBuildManifest.ts imported expo-constants at module scope,
and authClientMetadata imports that module, so every test reaching
authClientMetadata pulled in expo-modules-core — which reads React
Native's `__DEV__` global as an import side effect. vitest does not
define it.
That module's own doc comment already stated the intent ("so the
version-formatting logic stays testable without pulling react-native
into the unit test environment"); the split just did not go far enough
to cover its own consumers. The manifest read is now function-scoped
and fail-soft: outside a real Expo runtime there is no manifest, and
"no manifest" already means "no SHA", which is what connection.test.ts
asserts (plain "1.0.4", no +bk suffix).
Also formats four files that were carrying pre-existing whitespace
drift from commits made outside this worktree, which the staged-files
pre-commit hook never saw. `vp fmt --check` is now clean across all
3350 files.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…gnostic Moving this module out of apps/web and into client-runtime subjected it to the package's Effect diagnostics, which reject `Date.now()` in favour of Effect's Clock. `nextPlanDiscussionId` is a client-side id generator called straight from a React event handler; threading a Clock through it would add real plumbing for no benefit, and the wall clock is only there to stop ids colliding across a reload (a bare counter restarts at 1 and would make `upsertDiscussion` edit an existing discussion instead of creating one). So it takes the same file-scoped opt-out with a stated reason that threadSettled.ts already uses for UI-level time. Worth recording why CI caught this and my local runs did not: `vp run typecheck` uses tsgo with these diagnostics, while a bare `npx tsc --noEmit` does not. Re-verified with `vp run typecheck` in all four touched packages (client-runtime, shared, mobile, web) — clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Problem
Mobile had no way to act on a proposed plan at all. The thread showed a "Plan Ready" pill and the only way forward was finding a desktop — there was no plan affordance anywhere in
apps/mobile.What this adds
A native plan-review surface reachable from the thread screen: read the plan, question specific lines, then approve or request changes.
Web reviews plans in a Plate rich-text editor, which does not port to a phone. The operations that matter on a phone are read, question, and decide — so this surface is read-plus-annotate, and reviewer edits to the plan body stay a desktop affordance.
editedMarkdownis always null on submit, whichPlanReviewSubmitInputalready allows.locateQuotedLineRange. A quote the agent has since rewritten is still listed as unanchored rather than silently dropped or pinned to the wrong paragraph.PlanReviewPanel, so the screen doesn't go stale when the agent revises the plan or another client comments.Fork-cost notes
Two upstream-owned files are touched, one marked line each:
ThreadRouteScreen.tsx— one import, one element.PlanReviewThreadBannerresolves its own capability, documents and visibility and returns null when inapplicable, so no conditional leaks into the upstream render.Stack.tsx— one route pair, reusing the diff-review composer's Android formSheet workaround.planReviewMarkdownmoved fromapps/web/src/components/planreview/intopackages/client-runtime/src/state/rather than being copied — it was already DOM-free and both clients need it, following the pattern set by a768835 for the phase sidebar. Git tracked it as a rename, so history follows.The fork-marker baseline shrank (169 → 168): marking the
Stack.tsxedit made that file fully compliant, and the checker requires removing it so the ratchet cannot loosen again.Verification
planReviewAvailability,planReviewDocumentModel,planReviewSelection), all in fork-owned files so they prove the feature survived the next upstream merge.tsc --noEmitclean: mobile, web, client-runtime, shared.vp lintclean on the new scope.scripts/check-fork-markers.tspasses.No screenshots: I have not exercised this on a simulator. It is behind the
planReviewcapability, so it stays invisible on any server that does not advertise it.Model: Claude Opus 5 (1M context), harness: Claude Code in T3 Code.
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Merged
expbkmainin, resolving one relocation conflictThis branch also carries two commits that predated my work (
a768835c4phase-sidebar sharing,d14436353branded mobile build) — no other open PR claims them, so this PR is their delivery path.Merging
expbkmainhit one conflict inPhaseGroupedSidebar.logic.ts, and it is the relocation hazard AGENTS.md calls the most dangerous kind rather than an ordinary content conflict:a768835c4(ours) moved that logic into@t3tools/client-runtime/state/phase-sidebar, leaving the Tailwind class-name helpers plus anexport *.317f7ce47(theirs, onexpbkmain) added to the same block in place.Taking ours drops the Mattermost feature; taking theirs re-duplicates logic that now lives in client-runtime. Resolution: keep the relocation and port their two additions into the module the logic moved to —
PhaseSidebarMattermostLink+resolvePhaseSidebarMattermostLink, andmattermostLinkSupportedonPhaseSidebarRow.I verified those were the only things
expbkmainadded to the block two ways: diffing its export set against the relocated copy, and reading every hunk in that file since the merge base.apps/webneeded no change — the existingexport *meansPhaseGroupedSidebar.tsximports both symbols from exactly the same path as before.Evidence the port is behaviourally correct:
MattermostThreadBadge.test.tsximportsresolvePhaseSidebarMattermostLinkthrough the web re-export path and its 6 tests pass unchanged.