Skip to content

fix: show resolved path in setup wizard's config location picker - #855

Open
kishore280 wants to merge 4 commits into
Nano-Collective:mainfrom
kishore280:fix/852-location-picker-show-path
Open

fix: show resolved path in setup wizard's config location picker#855
kishore280 wants to merge 4 commits into
Nano-Collective:mainfrom
kishore280:fix/852-location-picker-show-path

Conversation

@kishore280

Copy link
Copy Markdown

Fixes #852.

The location picker offered "Global user config" vs "Current project
directory" as bare labels, no path shown either way. No way to know
which directory you were about to write into before confirming.

Now shows the resolved path next to both options, home-relativized
with ~, truncated on narrow terminals using the same
PATH_LENGTH_NARROW_TERMINAL/PATH_LENGTH_NORMAL_TERMINAL constants
status.tsx already uses for this.

Ran: test:ava on location-step.spec.tsx (16 pass, 1 new), test:types, test:lint.

@kishore280

Copy link
Copy Markdown
Author

@akramcodez kept this minimal on purpose. Just added the resolved file path next to each option in the location picker so you can see where it's about to write before confirming, e.g.:

> Global user config  (~\AppData\Roaming\nanocoder)
  Current project directory  (...\Microsoft VS Code Insiders)

Didn't touch the UI beyond that — if no provider's configured, the chat panel still just sits stuck on "Loading models..." with no real feedback. Left that out of scope here, happy to pick it up separately if any UI changes are planned down the line.

@will-lamerton

Copy link
Copy Markdown
Member

Hey @kishore280 - thanks for this, the direction is right and the changeset/scope are clean. Two things I'd like fixed before merge:

  1. homeRelative uses an unanchored prefix check, so a sibling home directory gets mangled: with home /Users/will, the path /Users/willy/projects/app renders as ~y/projects/app. Needs a separator boundary, i.e. resolved === home || resolved.startsWith(home + sep).

  2. The truncation budget doesn't account for the label prefix, and it double-truncates. "Current project directory (" is already 28 chars, and StyledSelectInput renders items with wrap="truncate-end" while truncatePath keeps the end of the path. So on an 80-col terminal a deeper path comes out as Current project directory (...tHub/some-org/some-really-long-monorepo-name/pa, cutting off the leaf directory, which is the thing the issue asked to see. Deriving the budget from actualWidth - label.length would fix it, or render the path on its own dim second line via a custom itemComponent.

Smaller points:

  • dirname(join(projectDir, configFileName)) is just projectDir, same for the global one, so the dirname/join round-trip can go.
  • app-container.tsx:58 already does home-relativization with configPath.replace(homedir, '~'). Worth pulling one shared helper into source/utils/ so the boundary fix lives in one place.
  • The new test's /Current project directory.*project/ matches almost anything with "project" in it. Asserting the concrete expected string would be stronger, and exporting homeRelative (with an injectable home, like daemon/install.ts does) would let you cover the case in point 1.
  • The double space before ( doesn't align anything since the two label stems are 18 vs 25 chars.

@will-lamerton

Copy link
Copy Markdown
Member

@kishore280 thanks for the quick turnaround. Point 1 and all the smaller points are sorted: the separator boundary fix looks right, the shared helper in source/utils/path.ts is the right home for it, app-container.tsx now goes through it, homeRelative is exported with an injectable home and has the sibling-directory case covered, and the dirname/join round-trip is gone.

Point 2 isn't fixed yet though. Rendering the path on its own line was the right call, but the budget still doesn't match the space the row actually has, so the original symptom still reproduces.

location-step.tsx:143 uses Math.max(10, Math.min(76, actualWidth - 4)), and actualWidth is raw process.stdout.columns. But LocationStep renders inside base-config-wizard.tsx:393's TitledBoxWithPreferences, which costs 2 columns of border plus 4 from paddingX={2}, then the select's indicator takes 2 and the item's own indent another 2. Actual room for the path is boxWidth - 10, not actualWidth - 4. So truncateMiddle sizes to 76, and Ink's wrap="truncate-end" then clips the overflow off the end - same double-truncation as before, just moved.

I rendered the step inside the real wizard container with a deep project dir:

60 cols:  /Users/will/Documents/GitHu...epo-name/packag…
80 cols:  /Users/will/Documents/GitHub/some-org...long-monorepo-name/packag…
120 cols: /Users/will/Documents/GitHub/some-org/some-really-long-monorepo-name/packages/leaf-dir

packages/leaf-dir is the thing #852 asked to see, and it's gone at 80 cols.

boxWidth is already returned by the hook and fixes it at every width. I checked this:

const {boxWidth, isNarrow} = useResponsiveTerminal();
const pathBudget = Math.max(10, boxWidth - 10);
60 cols:  /Users/will/Documents/...ame/packages/leaf-dir
80 cols:  /Users/will/Documents/GitHub/som...monorepo-name/packages/leaf-dir

That also drops the 76 cap, which was truncating paths that had room to fit on a wide terminal.

One new thing while I was in there: LABEL_PATH_SEPARATOR = '\u0000' isn't needed. ink-select-input renders itemComponent as <ItemComponent {...item} isSelected={...} />, so the whole item is already spread into it - add a path field to LocationOption and read it straight off the props, widening StyledSelectInput's itemComponent type to carry it. As written, const [stem, path] = label.split(SEP) types path as string but yields undefined if the separator is ever missing, and truncateMiddle would throw on that; it's only safe today because both labels are hardcoded.

Smaller points:

  • 10, 76 and 4 are inline in the render path while PATH_LENGTH_NARROW_TERMINAL / PATH_LENGTH_NORMAL_TERMINAL sit in source/constants.ts:76. The PR description still says those constants are used, but the final code doesn't use them - worth either using them or updating the description.
  • truncateMiddle overlaps with truncatePath in useTerminalWidth.tsx:130, which status.tsx still uses. The keep-both-ends strategy is genuinely different and justified, so no need to merge them, but a one-line comment on truncateMiddle saying when to prefer it would stop the next person picking the wrong one.
  • The new assertion in location-step.spec.tsx compares against homeRelative('/test/project'), which is an identity call for that input, so it's a little circular. A literal would be stronger. More importantly it renders LocationStep bare with a short path, which is why it doesn't catch the clipping above - a case that renders it inside the wizard container with a deep path would lock point 2 down.

Everything else passes on the branch (path.spec.ts, location-step.spec.tsx, useTerminalWidth.spec.tsx, 33 tests). Fix the budget and add the regression test and I think this is good to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Setup wizard writes config to cwd, not a fixed location — breaks VS Code extension connection

2 participants