fix(mobile): prevent wrapped composer text clipping - #4950
Open
iscekic wants to merge 39 commits into
Open
Conversation
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe latest commit moves the lock-contention test out of Files Reviewed (2 files in incremental range)
Verification notes
Assumption stated for transparency: the six Previous Review Summaries (8 snapshots, latest commit b23fabe)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit b23fabe)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe latest commit reverted the shared e2e harness back to base, but Overview
Issue Details (click to expand)WARNING
Files Reviewed (4 files in incremental range)
Verification notes
Assumptions stated for transparency: the six Fix these issues in Kilo Cloud Previous review (commit 263407e)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental range between the previously reviewed commit Files Reviewed (0 files)No files changed in the incremental range Verification notes
Assumptions stated for transparency: findings on files outside the incremental diff were not re-analyzed, per incremental review scope; the summary comment target could not be pre-read because that API call was blocked, so the trusted comment ID from the review context was used as-is. Previous review (commit c370dd8)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental range removes the three Files Reviewed (3 files)Incremental scope since
Verification notesPreviously reported findings, re-verified against HEAD
Additional checks on the incremental range:
Not applicable to this range: no Markdown, landing-page, or SEO-relevant files changed; no new subscriptions, timers, listeners, or retained references were introduced, so there is no memory-leak risk. Assumption stated for transparency: findings on files outside the incremental diff (including the Previous review (commit 278a536)Status: 5 Issues Found | Recommendation: Address before merge Executive SummaryThe latest commit reverted Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (5 files)Incremental scope since
Verification notes
Fix these issues in Kilo Cloud Previous review (commit c561135)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental scope since Files Reviewed (2 files)Incremental scope since
Previously reported issues resolved in this range
Verification notes
Previous review (commit 6204b9f)Status: 2 Issues Found | Recommendation: Address before merge Executive SummaryThe Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files)Incremental scope since
Previously reported issues resolved in this range
Verification notes
Fix these issues in Kilo Cloud Previous review (commit 154bb92)Status: 3 Issues Found | Recommendation: Address before merge Executive SummaryThe new Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files)Incremental scope since
Previously reported issues resolved in this range
Verification notes
Fix these issues in Kilo Cloud Previous review (commit fa5e8a0)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe permission-preserving [Snapshot truncated.] Additional previous summary content was truncated to keep this comment within platform limits. Reviewed by claude-opus-5 · Input: 28 · Output: 5.3K · Cached: 529.6K Review guidance: REVIEW.md from base branch |
…olerant flows - rewrite e2e/AGENTS.md in simplified technical English: task-structured, scripts own the mechanics, 410 -> ~215 lines - new e2e/github-stub.sh: PR-review stub server + env line + token seed as one reversible command pair; failed starts clean up after themselves - emulator-start --wait retries a failed launch once itself (boot-envelope timeout keeps the GPU, other failures switch to software rendering); the runbook's manual GPU decision table is gone - appium.sh hierarchy writes to a file and prints the path; a raw XML dump into an agent session is large enough to kill it silently - raise iOS cold-launch/settle budgets to 120s (timed out under parallel dual-bundle load); document ANR-under-load handling - simulator claims record originalDeviceName and bootedByClaim intent in the initial write, so a lost post-boot rewrite can no longer leak a renamed or still-booted device on release
- emulator-start retry: rethrow the already-exists guard (a retry would tear down a live emulator) and keep attempt 1's error when the interim teardown itself fails - github-stub: curl transport failures now route through seed_fail, so a failed start still rolls back the session and env line - github-stub: guard against a .env.local without a trailing newline before appending, so the marker line can never merge into (and later delete) an existing variable
- tmux targets use exact-match (=name): bare -t prefix-matches, so a sibling worktree whose slug extends ours could be probed or killed in its place - simulator claims write atomically (temp + rename/link): in-place truncation let an unlocked reader see a partial file, judge it corrupt, and delete a live claim - a claim abandoned after its boot (rename failure) now powers the device off, or keeps the claim when even that fails - simctl bootstatus is bounded (15 min) so a wedged CoreSimulator cannot block a claim forever - emulator-start tears down after a failed second attempt too - github-stub start claims its port machine-globally and verifies its own session survived, so two worktrees cannot share one stub silently; stop releases the claim - appium: driver install re-checks inside the install lock; server startup verifies port ownership before adopting a listener; stop escalates to SIGKILL instead of orphaning a live server - record.sh serializes start/stop per device with the shared process lock - open-app consumes its launch budget in 30s slices so an ANR dialog is answered promptly instead of after the full wait - login.sh matches the recipient literally, not as a regex - hierarchy mktemp template puts Xs last (BSD mktemp leaves embedded Xs literal, making every bare call after the first fail) - AGENTS.md: per-platform mixed fixtures, pull-and-grep for ADB dumps, stub-stop token residue note - mobile-workflow tests updated for the raised launch/settle budgets
- appium: the recorded-server fast path now verifies the recorded pid owns the listener before adopting (a recycled pid plus a sibling's server on the same port answered /status and got adopted) - github-stub: seeding extracted into seed_token; new 'seed <email>' subcommand adds a token row for a second signed-in account (the other platform's verifier) without touching the running stub - github-stub: the port claim covers only the choose-to-bind window and an EXIT trap releases it, so an interrupted start cannot leak claims and erode the port range - github-stub: githubUserId carries the pid — concurrent seeds in the same second cannot collide on the unique github_user_id - AGENTS.md: the ADB dump example creates its own mktemp file instead of assuming $SCRATCH
devSeedUserGithubToken can answer 200 with success:false (row not upserted); the status code alone reported a seed that never landed.
A process that survives even SIGKILL kept its state file removed and the stop reported success; the caller then released device claims over a live server. stop now fails and keeps the state in that case.
- claim_port reaps an orphaned port claim (older than 120s with nothing listening) and the start path traps INT/TERM/HUP besides EXIT, so a killed start can no longer shrink the port pool permanently - githubUserId derives deterministically from the account email: the upsert's setWhere only updates a matching row, so the fresh random id made every re-seed of an already-seeded account report success:false and tear the stub down with a misleading hint - lsof ownership probes tolerate no-match exits; pipefail otherwise killed appium.sh before the fresh-start fallback could run
…ling - appium startup adopts a port only on proven ownership (listener pid equals ours); an empty lsof keeps polling instead of adopting a vanished or foreign server - github-stub signal traps exit instead of continuing a start that just released its port claim - success:false with 200 and no error means an account seeded under an older random id — keep that usable row instead of tearing the stub down
success:false can come from an older random-id seed or from a real GitHub connection on the dev account; the keep-message now says so instead of implying only legacy seeds, and notes the stub accepts any token.
…lback - a kept token row is verified to be the stub's (getUserAuthorization githubLogin == kilo-stub-user); a real GitHub connection row fails with a drop-the-row hint — under the stub its refresh bypasses GITHUB_API_BASE_URL, hits real github.com, and revokes itself - start rolls back everything (session, env line, state, port claim) via one EXIT trap when it dies before the final state write, so an aborted start no longer blocks the next one behind 'session already runs' - appium ownership checks degrade to pid+status when lsof is missing or cannot attribute the socket, instead of killing healthy servers across 50 port blocks
- the row probe checks HTTP status and error envelope before advising: a failed probe reports 'could not determine the existing row' instead of confidently telling the operator to delete a row it never examined; a kept row must also be connected (a revoked stub row gets the drop hint) - start's rollback undoes only what this run did (session-created and env-added flags), so a start losing the session-creation race cannot kill the winner's session, env line, or state - the recorded-server reuse path applies the same blind-lsof rule as the start loop: adopt on pid+status after three blind probes instead of orphaning a healthy server and consuming a port block per invocation - drop dead SEED_FAIL_MODE and the stale random-id comment
- github-stub creates its session under a pid-unique name armed before the create and renames to claim the canonical one: a signal at any instant tears down only our own session, and a lost rename race touches nothing but our port claim; the env-added flag arms before the append for the same reason - the appium reuse probe rechecks pid liveness and /status on every pass, so a server dying mid-probe is cleaned up (stop_server) instead of adopted; a foreign listener still only drops our state
…windows Concurrent stub commands raced over the session name, env line, and state dir; each guard narrowed the window without closing it. The shared process-lock (the appium.sh pattern) removes the class: one stub command per worktree at a time, so the entry check is authoritative, the rollback flags only order against signals, and the unique-name rename dance goes away.
Both row branches run after the account answered the seed call, so the sign-in hint contradicted what the script had just proved.
- every network call under the stub lock carries --max-time 30, so a hung local API cannot hold the lock to its 300s wait bound; status runs unlocked (read-only) and answers during a long start - a failed session create (a survivor of a SIGKILLed lock wrapper got there first) resets the created flag before exiting, so cleanup cannot touch the survivor's session - align the env-append comment with the lock (the rename step it cited is gone)
pnpm dev:seed app:github-integration-copy <email> finds the newest real integration in the shared dev database (fakes excluded: kilo-stub* logins and far-future expiries — a real refresh token expires within months), decrypts it with git-token-service's local dev key, re-encrypts for the target account under a synthetic github_user_id (the pair index is unique), and upserts. Scenarios that need a real integration (cloud agents) run against the copy; with no valid donor the command fails with an explicit BLOCKED message, and the runbook tells verifiers to report VERIFICATION BLOCKED rather than fake or silently skip. Verified against the dev db: copy mechanics end to end, and the blocked path on a database holding only seeded rows.
- donor filter excludes 13-digit synthetic github_user_ids (stub seeds and earlier copies): revocation lands only on the real row, so a surviving copy could hand out dead credentials as a fresh success - app:github-integration-copy --remove <email> deletes the account's token row, so an account can return to stub use; runbook and usage text now require separate accounts for stub and integration scenarios - verified live: remove clears the copied row and is idempotent
One instruction per sentence in the integration and stub sections; no rule changed meaning.
- the donor's access token is checked against api.github.com before the copy: database metadata cannot prove a rotated or GitHub-revoked token dead, so silent dead-credential copies reported success - the copy row carries a dummy, already-expired refresh token: GitHub refresh tokens rotate on use, so a refreshing copy would kill the donor's stored refresh token and poison every later copy - usage and runbook text updated to the access-token-only contract
The copy relocates real credentials, so every destructive path is now guarded and every shared-grant hazard is stated: - assertLocalDatabaseTarget: USE_PRODUCTION_DB=true and non-loopback hosts are refused, making the dev-only claim enforceable (verified live) - --remove deletes only a synthetic row (copy or stub seed) and names what it removed; a real connection is refused with instructions - a copy refuses to overwrite a real connection on the target account - donor lookup also matches google_user_email (normalized_email is nullable) - usage and runbook state the real blast radius: the copy shares the donor's OAuth grant, so disconnect on a copied account revokes the DONOR's authorization and no re-copy repairs it — disconnect scenarios are blocked on copied accounts; commit attribution carries a synthetic id - stub status reports 'starting' when a session has no state yet, instead of claiming up for a stub that is not usable - lock-bounded curls raised to 120s (30s was the budget this PR already found too small under full parallel load) - a failed tmux create distinguishes a concurrent session from a tmux error
A dead database dumped a driver AggregateError stack trace, and a dead network read as 'no valid donor'. Both now fail with one line naming the command that fixes them, so an agent does not spend a round guessing at ECONNREFUSED output or chasing a false BLOCKED. Verified live with postgres stopped.
- the synthetic-only rule is re-asserted in the upsert's setWhere, so a real OAuth callback landing between the pre-check and the write cannot be overwritten; zero updated rows reports that explicitly (postgres predicate verified against synthetic and real id shapes) - donors need >10 minutes of access-token life (twice git-token-service's 5-minute refresh buffer): a token inside the buffer passes the live probe, then triggers a refresh the copy cannot perform - the target lookup tries the exact sign-in email before the normalized fallback, since normalized_email is not unique
…dget - the 'can never refresh' claim was wrong: git-token-service gates refresh on the ACCESS token's expiry, so the dummy refresh token IS sent and the copy ends up revoked. Docs, usage text and the runbook now describe that end state, the tool reports usableUntil, and donors need 35 minutes of access life so a copy is usable for at least 30 - --remove reports what actually went away (.returning()), so a row replaced mid-command cannot read as removed - stub lock --wait raised to 900s, above the ~8 minute worst-case hold the 120s per-call budgets allow, so a queued stop or seed waits instead of failing - drop the now-unused 'or' import
The 35-minute SQL margin is measured before up to ten 15-second GitHub probes, so a copy could return with less than the promised 30 usable minutes. The selected donor is now re-checked against the clock after its probe, and one helper owns the usable-until arithmetic the filter, the check, and the report all share.
A campaign verifier died with no sentinel right after a doctor --json dump (the whole environment, PATH included). The runbook now states the general rule — bound anything that can print a lot — and names --json diagnostics alongside the Appium and hierarchy cases it already covered.
Contributor
Author
|
(bot) Kilobot review not detected after two retriggers, proceeding further |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Author
|
(bot) Correction to the resolved Appium thread: direct inspection confirmed bare |
iscekic
force-pushed
the
composer-wrap-fix-2ec5
branch
from
August 2, 2026 13:27
c370dd8 to
263407e
Compare
PR #4960 carries these changes and merges first. This branch keeps no copy. - dev/local/mobile-android.ts - dev/local/mobile-simulator.ts - dev/local/tmux.ts - dev/seed/app/github-integration-copy.ts
| assert.equal(readGradleWrapperVersion(root), '8.14.3'); | ||
| }); | ||
|
|
||
| test('lock contention on Android emulator launch throws the error message the retry guard exempts', async () => { |
Contributor
There was a problem hiding this comment.
WARNING: Leftover test from the reverted e2e harness — the retry guard it names no longer exists
chore(dev): revert shared e2e harness files (b23fabe) restored dev/local/mobile-android.ts to base, removing the emulator-start relaunch logic that exempted locked by another live process from retry. This test survived that revert, so:
- Its name asserts a contract ("the error message the retry guard exempts") that no code in
dev/local/mobile-android.tsimplements anymore — a future reader will believe the emulator relaunch policy is covered when it is not. - It exercises only
withProcessLockAsyncfrom./process-lock, so it belongs in a process-lock test, not inmobile-android.test.ts; the newimport { withProcessLockAsync } from './process-lock';on line 21 exists solely for it. const holder = await withProcessLockAsync(...)on line 379 binds a value that is never read (the callback returnsvoid), which is dead code even thoughdev/is outside thepnpm lintandpnpm typecheckscopes.
Given the PR is now scoped to the mobile composer width fix, dropping this test and its import returns dev/local/mobile-android.test.ts to base along with the rest of the harness revert.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
The reverted e2e harness removed the emulator-start retry guard, so the test name in mobile-android.test.ts was wrong. The test covers process-lock, not Android. Move it to process-lock.test.ts and rename it.
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
1 participant
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.
Summary
What
Why
The hidden measurement text was 2px wider than each real text area. At a wrap boundary it could undercount a line, leaving text clipped while scrolling stayed disabled.
How
w - 34(session composer) andw - 26(kilo-chat composer) helpers with arithmetic regression tests.node/react-test-renderer) and do not run Yoga or fireonLayout; iOS E2E covers real rendering.Verification
pnpm formatpnpm typecheckpnpm lintpnpm check:unusedpnpm test— 296 files, 2,582 testsgit diff --checkVisual Changes
After-only screenshots: the before state was not pursued because the 2px boundary is input/font dependent and the root cause was already fully traced in code.
Reviewer Notes
new-session-prompt.tsxis intentionally unchanged: its layout geometry is already correct.