feat(ui): switch projects from the panel header - #20
Conversation
The header's first line showed the open project's name as static text. It is now the control that changes the project: a dropdown listing recent projects, current one first with a checkmark and its parent directory underneath (home collapsed to ~), plus "Open Project..." below a divider. Selecting a row calls ProjectDataProvider.selectProject, the same host call the top bar's own project menu makes, so the switch belongs to the WINDOW rather than to this panel: the top bar name, the sidebar panels and every project-scoped tab follow it. Re-picking the open project is a no-op instead of re-running the host's project-selection flow to land where we already are. Two defects on the picker path, both of which made a folder chosen from "Open Project..." appear to do nothing: - The picker ran from inside the menu's own composition. Under HARDWARE rendering the menu is a heavyweight always-on-top AWT window (BossWindow provides LocalHeavyweightOverlays = true), and the host's picker parents its native FileDialog to KeyboardFocusManager.activeWindow. Dismissing the menu and opening the picker in one event therefore handed the modal dialog an owner that the same frame disposes. The pick request now crosses back into the panel's own composition and runs a frame later, once the menu window is gone. - The macOS picker returns dialog.directory WITH a trailing separator when nothing inside the folder was selected. PathUtils.name of that is "", so such a pick reached the host as a project literally named "Unknown", on a path that did not match the recents entry for the same folder. New PathUtils.trimTrailingSeparator runs first; the dropdown's dedupe key shares the same helper, so one folder cannot occupy two rows. The picker path is logged at INFO throughout. It is driven by a native dialog inside the host, so without those lines there is no way to tell a cancelled dialog from a missing provider from a callback the host ignored. Tests: 19 new (dropdown ordering and matching incl. Windows separators; picker result handling). slf4j-simple joins the test runtime because BossLogger binds slf4j in its static init, so any test reaching a log call died without it.
Review:
|
|
@claude Please review this PR against its latest head commit. Check correctness, regressions, security, and test coverage, and report any actionable findings so they can be fixed before merge. |
Review:
|
|
@claude Please review this PR against its latest head commit. Check correctness, regressions, security, and test coverage, and report any actionable findings so they can be fixed before merge. |
Review:
|
Review: switch projects from the panel headerSolid work. The two picker defects are well diagnosed, the description explains why Convention checks all pass:
Findings below, roughly in the order I would act on them. I could not compile locally (no network for the API jar), so this is from reading rather than running. Nothing blocks; 1, 3 and 4 are what I would fix before merge. 1. The 250 ms burst never exits early, and samples across the process boundary on the UI dispatcher
LaunchedEffect(recents, projectRefreshRequest) {
repeat(20) {
refreshProjectPath()
delay(250L)
}
}All 20 iterations run unconditionally, including after the path has already changed, so one project switch costs 20
4 calls/second on the UI dispatcher is a real step up from that. Two cheap fixes, both worth doing: fun refreshProjectPath(): Boolean {
val current = getProjectPath()
if (current == projectPath) return false
projectPath = current
gitViewModel.onProjectChanged()
searchViewModel.clear()
return true
}
LaunchedEffect(recents, projectRefreshRequest) {
repeat(20) {
if (refreshProjectPath()) return@LaunchedEffect
delay(250L)
}
}plus 2. The picker burst is spent while the dialog is still open
onOpenProject = {
projectSelector.pickDirectory()
projectRefreshRequest++
},
3.
|
ProjectSwitcher.kt |
token |
|---|---|
BorderColor.copy(alpha = 0.45f) (:109, :280) |
CodebasePalette.Hover (= BossColors.contextMenuHover) |
Surface(color = BossThemeColors.SurfaceColor) (:178) |
CodebasePalette.Raised (= BossColors.contextMenuBackground) |
BorderStroke(1.dp, BossThemeColors.BorderColor) |
CodebasePalette.BorderStrong (= BossColors.contextMenuBorder) |
14.sp (:124), 13.sp (:233, :264), 10.sp (:242) |
CodebaseMetrics.PrimaryText / SecondaryText / MetaText |
MenuRowHeight = 34.dp (:55) |
CodebaseMetrics.RowHeight (22.dp) / TouchTarget |
Modifier.size(18.dp) (:119), 16.dp |
CodebaseMetrics.Glyph (14.dp) |
:124 is also an undocumented visual change: the project name went from CodebaseMetrics.SecondaryText (12.sp, SemiBold, letterSpacing = 0.2.sp) to a bare 14.sp Medium. That is a noticeable bump on the first line of a panel whose list rows are 22 dp, and the description does not mention it. If intentional, say so; otherwise it is an accidental regression in header density.
Smaller point while you are in there: the header row now carries Icons.Rounded.Inventory2 and the switcher Icons.Outlined.FolderOpen side by side. Two container/folder glyphs on one line read as noise; one can go.
4. Dedupe and current-project matching are case-sensitive
ProjectSwitcherEntries.matchKey normalizes trailing separators only. On Windows, and on the default case-insensitive APFS volume on macOS, C:\Dev\Boss and C:\dev\Boss are the same directory, but they occupy two rows and neither gets the checkmark. Same class of bug as the trailing-separator one you just fixed, and build already takes an injectable separator, so it is testable on any OS:
private fun matchKey(path: String, separator: Char): String =
PathUtils.trimTrailingSeparator(path, separator)
.let { if (separator == '\\') it.lowercase() else it }Or normalize once at the host boundary. Either way the existing windows paths dedupe and match on their own separator test is the natural place to pin it.
5. pickPending as a Boolean can leave "Open Project..." permanently dead
ProjectSwitcher.kt:83-97. Cancellation is handled (finally runs) but withFrameNanos { } at :91 suspends until the composition frame clock ticks. If that clock is ever paused while the effect is live, the coroutine neither completes nor cancels, pickPending stays true, and since LaunchedEffect(pickPending) restarts only on a value change, every later click sets true -> true and does nothing, not even a log line. A monotonic token sidesteps the question:
var pickRequest by remember { mutableStateOf(0) }
LaunchedEffect(pickRequest) {
if (pickRequest == 0) return@LaunchedEffect
withFrameNanos { }
logger.info(LogCategory.FILE, "Dispatching deferred project picker after popup dismissal")
onOpenProject()
}Low probability, but the failure mode is "the row is dead until the panel is reopened", the exact symptom this PR set out to kill.
6. CodebaseViewModel.selectProject is dead code
CodebaseViewModel.kt:568 has no callers: the header goes through the ProjectSelection in CodebaseComponent, and the FILES empty state only uses pickDirectory(). Drop it, or route the header through the ViewModel and drop the second ProjectSelection.
Relatedly there are now two ProjectSelection instances per panel. It is stateless so nothing breaks, but the empty-state button and the header row reach the same host call by two different paths; hoisting one would make the boundary singular, which was the stated point of extracting the class.
7. Accessibility / keyboard
Acknowledged as untested, so just naming the gaps: menu rows are clickable(indication = null) with no focus ring and no arrow-key traversal, so the menu is mouse-only. And contentDescription = "Switch project" sits on the chevron (:134), so a screen reader announces it on a decorative glyph while the row is the actual button. contentDescription = null on the chevron, plus a semantics label and stateDescription for expanded/collapsed on the trigger Row, would be more correct.
8. Logging volume
Every selection logs an absolute path at INFO. The justification for landing it that way is sound, but once confirmed in the field consider dropping the success-path lines to DEBUG and keeping WARN for the failure branches. INFO on a hot user action fills the host log, and absolute paths are about the most user-identifying strings this plugin handles.
Follow-ups, out of scope here
BossPopup/BossPopupAnchoringare new to this plugin andplugin.jsonkeepsminApiVersion: 1.0.87. CI building against the released 1.0.87 jar should cover it, but worth confirming explicitly that both symbols exist at 1.0.87 and not in a later API, since the manifest is what the host gates loading on.- By this PR own JxBrowser reasoning, the three Material
DropdownMenucall sites inGitTabUi.kt(:682,:793,:1371) carry the same latent draw-behind-the-page bug that motivatedBossPopuphere. Worth a tracking issue. - Geometry note on
BossPopupplacement: the zero-size probe sits inside the innerColumnof the header, between the name row and the pathText. SoAnchorBoundsanchors below the name, not below the whole header, and the menu should overlap the home-collapsed path line rather than clearing it. If that is what you saw and accepted, fine; if the intent was "under the header", the probe wants to be a sibling of the headerRow.
|
Merge-preparation handoff (implementation stopped at user request): current prepared head is The actionable findings in Claude’s last inspected review were addressed in |
Review: feat(ui): switch projects from the panel headerThis is a well-scoped PR and the description is very good. Both picker defects (the native dialog getting a disposed popup window as its owner, and trailing separators producing projects named "Unknown") are diagnosed precisely, and both fixes are small and land in the right place. Moving the picker and selection logic into BOSS conventions ✅
Suggestions1. The header now shows two icons (visual). 2.
I'd pass in 3. The tooltip covers the clickable trigger. 4. The confirmation burst can expire while the picker is still open. The same applies to the FILES empty-state button ( 5. Minor issues
PerformanceOpening the panel and every SecurityNothing of concern. Paths come from the host's picker, the host's recents list, or MCP arguments, and are only handed back to TestsThe tests are thorough for the pure logic: trimming, dedupe and ordering, null or throwing providers, MCP normalization, and the bounds and cancellation of the polling loop. Adding In short, the logic and tests look correct. I'd fix the duplicate icon and the token/sizing mismatch (items 1–2) before merging, since they are the visible problems in the redesigned header. The rest are polish. |
Review: feat(ui): switch projects from the panel headerThis is a solid PR. The pure parts are pulled out well ( BOSS conventions ✅
Potential issues / suggestions
Test coverageCoverage is good: normalization (including Windows separators, roots and meaningful trailing spaces), ordering and dedupe, the current path missing from recents, poll bounds and cancellation, async picker completion, provider failures, and MCP normalization. Small nits:
SecurityNo concerns. The MCP Overall this looks good to merge after a look at #1 and #2. Neither is a correctness blocker. |
Review: project switcher in the panel headerThis is a well-scoped PR. The two picker-path bugs are diagnosed clearly, and the comments explain why things are done a certain way, not just what the code does. I found no blockers. Notes below, most important first. ✅ BOSS conventions
Potential issues / suggestions
Performance
Security
TestsThe tests cover the pure logic well: path trimming including Windows drive roots and UNC, recents ordering and dedupe, polling bounds and cancellation, an async picker, and MCP normalization. Adding
Nice work overall. The header-as-switcher and routing through |
What
The header's first line showed the open project's name as static text. It is now the control that changes the project.
~), lazily rendered within a 360dp maximum viewport.Open Project...below a divider, wired to the picker the empty state already used.ProjectDataProvider.selectProject, the same host call the top bar's project menu makes, so the switch belongs to the window: top bar name, sidebar panels and project-scoped tabs all follow it. Re-picking the open project is a no-op rather than re-running the host's project-selection flow to land where we already are.BossPopup, not a plain ComposePopup: this panel sits beside browser tabs and Chromium composites over the Compose scene. Placed after the trigger row in aColumnsoAnchorBoundsopens the menu under the header rather than over it.Two defects on the picker path
Both made a folder chosen from
Open Project...appear to do nothing.The picker ran from inside the menu's own composition. Under HARDWARE rendering the menu is a heavyweight always-on-top AWT window (
BossWindowprovidesLocalHeavyweightOverlays = true), and the host's picker parents its native dialog to whatever window is active:Dismissing the menu and opening the picker in one event handed that modal dialog an owner the same frame disposes. The pick request now crosses back into the panel's own composition and runs a frame later, once the menu window is gone.
Trailing-separator paths named every such project "Unknown". macOS returns
dialog.directorywith a trailing separator when nothing inside the folder was selected, andPathUtils.nameof that is"". Such a pick reached the host asProjectData(name = "Unknown", path = "/dev/BossTerm/")- not the folder's name, and a path that did not match the recents entry for the same folder. NewPathUtils.trimTrailingSeparatorruns first, and the dropdown's dedupe key shares the helper so one folder cannot occupy two rows.The picker path is now logged at INFO throughout. It is driven by a native dialog inside the host, so without those lines there is no way to tell a cancelled dialog from a missing provider from a callback the host ignored.
Validation
The PR now integrates the merged Files/Search/Git panel redesign: the project switcher sits in the shared header above all tabs. Project selection and the picker share a lightweight service. Project changes are sampled on IO in bounded 250ms bursts that stop on confirmation, with a 5-second idle poll and serialized updates. Picker completion starts the burst after a valid folder is submitted, including the FILES empty-state action. The UI does not assume that a host confirmation prompt was accepted.
CI=true ./gradlew test buildPluginJar --console=plain -Pkotlin.daemon.jvmargs=-Xmx2gpassed against released API 1.0.87 (the declared minimum): 249 tests, zero failures/errors/skips. Regression tests cover path normalization, recents ordering/dedupe, provider failures, polling bounds/cancellation, asynchronous picker completion, and normalized MCP output. Independent review of the final fixes found no blocker. Provider read failures retain the last confirmed path and allow subsequent polling to recover; cancellation propagates. An unchanged initial composition skips the rapid burst.The native popup's frame-deferred picker ownership, visual geometry, and keyboard interactions are not covered by automated Compose tests. No BOSS app launch, installation, or hot-reload was performed during this merge preparation. Earlier manual validation predates the integrated header.