Conversation
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of the commits after Files Reviewed (14 files)
Previous Review Summaries (7 snapshots, latest commit 60f3398)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 60f3398)Status: No Issues Found | Recommendation: Merge Executive SummaryThe new commit keys both mobile text rules on the copy's script instead of the interface direction (a joined-script Arabic run resets letter spacing and drops the mono family in either direction, while Hebrew keeps its tracking on an English screen), and the in-app sign-out confirm and the direction-agnostic facing hit-slop cap still hold at HEAD. Files Reviewed (14 files)
Previous review (commit fdfa0e5)Status: No Issues Found | Recommendation: Merge Executive SummaryThe new commits since the prior review only merged Files Reviewed (9 files)
Previous review (commit 94e1090)Status: No Issues Found | Recommendation: Merge Executive SummaryThe follow-up commits stop the leaked mounted Files Reviewed (11 files)
Previous review (commit 38f23f9)Status: 2 Issues Found | Recommendation: Address before merge Executive SummaryThe joined-script tracking reset and the one-dialog sign-out change are correct and the previously reported RTL hit-slop overlap is fixed; the only remaining findings are a leaked test renderer and a now-inverted test comment. Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (17 files)
The branch was rewritten (rebased onto Fix these issues in Kilo Cloud Previous review (commit 7576aa4)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (11 files)
Re-reviewed incrementally after the branch was rewritten. The joined-script reset in Fix these issues in Kilo Cloud Previous review (commit 6170b07)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Re-reviewed incrementally: the branch was rebased onto a newer base, but the four changed files' content is unchanged from the prior review. The joined-script reset is prepended to the rendered style array while the caller's Previous review (commit 734a317)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
The joined-script reset is prepended to the style array while the caller's Reviewed by deepseek-v4.1-flash · Input: 0 · Output: 0 · Cached: 0 Review guidance: REVIEW.md from base branch |
45a951b to
6170b07
Compare
ae4e14d to
199cc17
Compare
2898e92 to
7576aa4
Compare
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
7e9efc8 to
ed9cbd2
Compare
ed9cbd2 to
38f23f9
Compare
…gs-such-as-and-t-5faea-6357
…gs-such-as-and-t-5faea-6357
…gs-such-as-and-t-5faea-6357
The joined-script test mounts a Latin tree and then an Arabic one through the module-level renderer, so the first tree stayed mounted: `afterEach` only unmounts the last one. The shared helper now unmounts the tracked tree before it replaces it, and the test reads the Latin styles before the Arabic mount takes its place.
…ces-ar-the-arabic-headings-such-as-and-t-5faea-6357
The main merge brought in the mono-variant tests, whose `mount(element)` helper this branch replaced with `renderRoot(element)` plus a `mount(children, style, className)` wrapper, so those calls no longer matched the signature. The mono tests now call `renderRoot`. `renderRoot` also unmounts inside the same `act` as the next `create`: assigning `renderer = undefined` between them narrowed it to `never` for the `if (!renderer)` guard and the following `renderer.root` read.
…ces-ar-the-arabic-headings-such-as-and-t-5faea-6357
…ces-ar-the-arabic-headings-such-as-and-t-5faea-6357
…ces-ar-the-arabic-headings-such-as-and-t-5faea-6357 # Conflicts: # apps/mobile/src/components/profile-screen.tsx
iscekic
added
merge-by-human
…ces-ar-the-arabic-headings-such-as-and-t-5faea-6357
eshurakov
left a comment
eshurakov
left a comment
There was a problem hiding this comment.
Warning — the letter-spacing fix this branch is named for is not present at the PR head.
apps/mobile/src/components/ui/text.tsx at head (89aa4e6) is byte-identical to main. The top commit 38f23f96 ("stop Latin tracking from splitting Arabic text") changed Text to reset via textLetterSpacing(props.children), i.e. by script regardless of interface direction, but that change is absent now — a later merge from main appears to have overwritten it.
Consequences at head:
containsJoinedScript/textLetterSpacing(apps/mobile/src/lib/rtl-text.ts:152,163) have no production caller (code search finds only their own definition and test).- The reset in
Textis still gated onI18nManager.isRTL(apps/mobile/src/components/ui/text.tsx:84-101), so a joined-script run in an LTR interface keeps the tracking class — the exact case38f23f96set out to fix. - The added tests only pin the interface-keyed behavior, so this regression is not caught.
If the interface gate is the intended behavior, the dead helpers and the commit message should be reconciled; if the script-based reset was intended, it needs to be reapplied.
…ces-ar-the-arabic-headings-such-as-and-t-5faea-6357
…rface The reset followed `I18nManager.isRTL`, so an Arabic label on an English screen kept the Latin tracking and split its joins. Key both script rules on the copy instead: - The letter-spacing reset follows `containsJoinedScript` (the joined Arabic blocks) in either direction. An RTL interface keeps resetting its RTL-script copy, because the RTL catalogs carry no tracked design the Hebrew block could take. - `withoutMonoFamily` follows `hasRtlScript` (Hebrew and Arabic) in either direction: JetBrains Mono ships no glyph of either script. `NATURAL_LETTER_SPACING` replaces the direction-named `RTL_NO_LETTER_SPACING`. `JOINED_SCRIPT` now covers Arabic Extended-B, the block `hasRtlScript` already read for the font rule. Delete `textLetterSpacing`, which no production code called since the reset became script-keyed.
|
Your warning was valid: the merge took Resolution in 60f3398, which does not re-apply the old helper:
|
Two RTL assertions outside this change — `chat-composer.test.ts` and
`new-session-prompt-initial-prompt.test.ts` — pin the array
`[{ writingDirection: 'rtl' }, undefined, undefined]`. A filtered array
dropped the trailing entry and failed both. Restore the array `main` ships:
the RTL run keeps its three entries, and a joined-script run in an LTR
interface carries a leading `undefined`.
…ces-ar-the-arabic-headings-such-as-and-t-5faea-6357
2 participants
Changelog for users
Changelog for maintainers
I18nManager.isRTL:withoutMonoFamilyfollowshasRtlScript(Hebrew and Arabic), and the letter-spacing reset followscontainsJoinedScript(the joined Arabic blocks), in either direction.NATURAL_LETTER_SPACINGreplaces the direction-namedRTL_NO_LETTER_SPACING; the reset is no longer direction-specific.JOINED_SCRIPTnow covers Arabic Extended-B, the blockhasRtlScriptalready read for the font rule. Without it the new LTR path would miss a block the Arabic catalog uses.textLetterSpacing, which no production code called since the reset became script-keyed.Text's two predicates first, thenJOINED_SCRIPT, then the changed expectations in the mounted tests, then the hit-slop constants and the sign-out paths.Verification
apps/mobilevitest runon the seven files this change touches: 140 tests pass. The files aretext.mounted,text.rtl-labels,text.rtl-tracking,rtl-text,section-header.mounted, and the two RTL counter tests the array shape pins (chat-composer,new-session-prompt-initial-prompt).it.each([false, true]); the mono-family rule is pinned insection-header.mountedandtext.rtl-labels.tracking-[1.5px]compiles to a positive value while the resolved spacing is 0 (compiledLetterSpacingintext.mounted).oxlinton the changed files: 0 warnings, 0 errors.oxfmt --list-differenton the changed files: no output.ccfea7d8a1: 18 checks pass, 11 skip, 0 fail. Thetestandtest (kilo-app)jobs cover the mobile suite;typecheck,lint,format-check,check-unused,drizzle-check,trufflehog, andKilo Code Reviewpass.60f3398484failedtest: a filtered style array dropped the trailing entry thatchat-composer.test.tsandnew-session-prompt-initial-prompt.test.tspin in RTL.c7a6e9c744restores the three-entry arraymainships.mainateba9bec1b8(merged twice, no conflict in either merge).E2E proof
The captures below predate this change. They prove the RTL screens and the sign-out flow on Android. No capture shows an Arabic label on an English screen, which is the case this change adds.
Open findings (not fixed here)
mainbefore this branch (#6497, then#6495). The earlier revision of this pull request re-applied that older helper and lost the newest commit in the merge. This revision keys both rules on the script and keeps one source for the reset.tsgo --noEmitin this worktree reports 106 errors in files this change does not touch (generated tRPC types are absent). CI holds the typecheck.Owner request