Repository navigation
refactor: small cleanups to the font install, auto-align loop, GUID and root dir widgets - #809
Merged
Merged
Conversation
After the clone, installPowerlineFonts either runs install.sh (macOS and Linux) or copies the font files (Windows), and each branch returns its result or throws. The "Platform-specific installation not implemented" result after them could never be returned, and an unsupported platform already returns its own message before the clone.
…while loop The padding pass jumped its for-loop counter past each merge group from inside the body, so the loop header no longer said how it stepped. A while loop now sets the next widget explicitly: the following one, or the one after a merge group once the group is padded. The output is unchanged. Two new tests pin the cases that depend on the stepping and had no test: an auto-align exclusion on a widget merged into the previous one is ignored and the columns after the group are still padded, and a line rendered without column widths keeps its natural widths.
generateGuid dropped the fraction of Math.random() * 16 with a bitwise "| 0". Math.trunc says that directly. For the values this sees, [0, 16), both give the same digit: they differ only past 2^31, for NaN and for Infinity, none of which Math.random() * 16 can produce. New tests pin the UUID v4 shape and the digits Math.random() at 0, 1/16, 1/2 and its largest value turn into, including the variant digit's 8 to b range.
Git Root Dir and JJ Root Dir stripped trailing separators from the root path, then split it on separators and dropped the empty parts. The split already drops the empty parts trailing separators leave, so the strip changed nothing: - With a name in the path, both give the same last part. - With no name, as for "/", the strip left an empty string and the code fell back to the original path, which is what the fallback now uses directly. The strip's pattern also rescanned a long run of separators from every position in it: 50,000 slashes before a name took 1.6s under Bun and 4.0s under Node. git and jj never print such a path, but the split alone does it in about a millisecond. An exhaustive check over every string of up to 8 characters drawn from "/", "\", "a", ":", " " and a newline gives the same name both ways. New tests cover trailing Windows separators in both widgets, "/" for JJ Root Dir and a path of separators alone for Git Root Dir.
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.
What
Four small cleanups. None of them changes what the status line or the TUI shows.
forcounter from inside the loop body.generateGuiddrops the fraction of its random digit withMath.truncinstead of| 0.I also went through the regular expressions that can take quadratic time on long input. The two in the root dir widgets go with the strip above. The others stay, with the reason for each under How.
Why
Font install. After the clone,
installPowerlineFontseither runsinstall.sh(macOS and Linux) or copies the font files (Windows). Each branch returns its result or throws. An unsupported platform gets its own message before the clone. So thePlatform-specific installation not implementedresult after the two branches could never be returned. TypeScript accepts the function without it:bun run lintis clean.Auto-align loop. The padding pass in
renderPowerlineStatusLineusedfor (let i = 0; ...; i++), then seti = jinside the body to jump past a merge group. A reader had to find that assignment to know how the loop moved. The jump matters in one case: an auto-align exclusion on a widget merged into the previous one. That exclusion is ignored because the loop never visits the widget. No test covered that case.generateGuid.(Math.random() * 16) | 0andMath.trunc(Math.random() * 16)give the same digit for every value in[0, 16). I compared them on 1,000,035 values ofMath.random()(0, the smallest positive number, the bottom of each of the 16 steps and a value just below its top, the largest valueMath.random()can return, and a million random draws), under Bun 1.4.2 and Node 26.10.0, and got 0 mismatches. They only differ for valuesMath.random() * 16can't produce:| 0Math.trunc-0.50-0(prints as0)2 ** 31-21474836482147483648NaN0NaNInfinity0InfinityRoot dir widgets. Both widgets did this:
filter(Boolean)already drops the empty parts that trailing separators leave, so the strip didn't change the name. The fallback only runs when no part is left. That happens only for a path made of separators alone, such as/, and then the strip had produced''and the code fell back torootDiranyway. I checked every string of up to 8 characters drawn from/,\,a,:, space and newline (2,015,539 strings): the old and new code give the same result for all of them, under Bun and Node.The strip's pattern was also the slow one here. With a long run of separators it starts a match at every separator in the run and scans to the end of the run each time:
/thenxgit and jj never print such a path. The change is worth making because the code gets shorter.
The other patterns. Each one can go quadratic on a crafted 50,000-character string:
/\.?0+$/number-format.ts1., 50,000 zeros,1/^hostname\s+(.+)$/igit-review-cache.tshostname, 50,000 spaces,a\rb/[\\/]+$/App.tsx,global-package-manager.ts/thenx/\/+$/global-command-resolution.ts,global-package-manager.ts/thenx/(\d+)\s+insertions?/(anddeletions?)git.ts,jj.ts/(\d+) insertion/(anddeletion)JjChanges.ts/\s*\(.*\)$/Model.ts(None of them gets input like that in practice. Where the input has a hard limit, here is the pattern at that limit:
/\.?0+$/toFixedoutput: at most 29 characters (a minus sign, 21 digits, the point and 6 decimals), sincedecimalsis capped at 6/[\\/]+$/PATH_MAX)/[\\/]+$/PATH_MAX)/(\d+)\s+insertions?/How
powerline.ts: removed the unreachablereturnafter the platform branches ininstallPowerlineFonts.renderer.ts: the padding pass is now awhileloop. Each turn setsnextto the following widget. When the widget heads a column that has a width,nextis set past the widgets merged into it instead. The loop moves tonextat the end of the turn. The rest of the body is unchanged. The oldif (!element) continue;is nowelement?.in the exclusion check andelement &&in the column check, so a missing element still just moves on.guid.ts:Math.truncin place of| 0.GitRootDir.ts,JjRootDir.ts: splitrootDirdirectly and fall back torootDir. A comment says why there is no strip.renderer-exclude-auto-align.test.ts: an exclusion on a widget merged into the previous one is ignored, and the columns after the group are still padded. A line with no column widths keeps its natural widths.guid.test.ts(new file): the UUID v4 shape, and the exact GUID thatMath.random()at 0, 1/16, 1/2 and its largest value gives.JjRootDir.test.ts: trailing Windows separators and/.GitRootDir.test.ts: trailing Windows separators and a path of separators alone.Left as is:
calculateMaxWidthsFromPreRenderedmoves its loop counter the same way. Another branch of mine rewrites that loop as awhileloop, so this one doesn't touch it.number-format.ts/\.?0+$/: it only seestoFixedoutput of at most 29 characters. A bounded form needs an alternation with a lookbehind, which is harder to read than the line it replaces.git-review-cache.ts/^hostname\s+(.+)$/i: it reads localssh -Goutput, which puts one space after the keyword. It is only slow with a long run of spaces followed later by a lone line terminator such as\r. The host taken from the remote URL can't carry that: ssh refuses it withhostname contains invalid characters. Only a quotedHostnamevalue in an ssh config file can, and that has no length limit (OpenSSH 10.3 prints one with 50,000 leading spaces).App.tsxandglobal-package-manager.ts/[\\/]+$/, andglobal-command-resolution.tsandglobal-package-manager.ts/\/+$/: these strip trailing separators from install directories andPATHentries in the user's own environment. They are only slow for a run of thousands of separators. A bounded form needs a lookbehind or a loop, and neither is simpler than the one-liner.git.ts/(\d+)\s+insertions?/anddeletions?:git diff --shortstatprints one summary line, with numbers of a few digits.jj.ts/(\d+)\s+insertions?/anddeletions?: injj diff --statoutput, a long run of digits can only come from a file name, which is at most 255 bytes. Adding(?<!\d)would bound the pattern, but it costs a lookbehind and no real input needs it.JjChanges.ts/(\d+) insertion/anddeletion: these only read the summary line ofjj diff --stat.Model.ts/\s*\(.*\)$/: it reads Claude Code's model display name, which is a few dozen characters. Another branch of mine replaces this line with a string search.Demo
None: nothing visible changes, and the built CLI prints byte for byte what
mainprints in plain and Powerline mode, also for a merge group whose second widget is excluded from auto-align (see Testing).Testing
mainas of fix(powerline): keep the Custom theme when Powerline is enabled again #793 (a0bedc0): it merges cleanly.bun run lintis clean, andbun testgives 4158 pass, 0 fail (4147 onmain). The built CLI under Bun 1.4.4 and Node 26.13.0 agrees across runtimes and matchesmainin plain and Powerline mode, and the TUI opens and exits cleanly under both. It adds no JavaScript or Node API newer than Node 14, the build's target.main's source, because they pin behaviour the refactors must keep. When I removed the merge-group skip from the loop, the new merged-exclusion test failed, and it was the only failure in the full suite.powerline-install.test.ts: 45 pass, 2 fail (main: 34 pass, 2 fail). The 2 failures are the same on both. They are the existing ESM spy failures inpowerline-install.test.ts(vi.spyOn(os, 'homedir')).distunder Bun 1.4.2 and Node 26.10.0:mainin plain and Powerline mode.