Skip to content

fix(herdr): align sidebar numbers with space shortcuts - #111

Open
barbatdev wants to merge 1 commit into
mainfrom
fix/herdr-sidebar-ordinals
Open

barbatdev wants to merge 1 commit into
mainfrom
fix/herdr-sidebar-ordinals

Conversation

@barbatdev

Copy link
Copy Markdown
Owner

Summary

Closes #109.

PR #108 is already merged, so this correction is a new focused PR rather than another PR on its old branch.

  • Derive sidebar rendering and shortcut targets from the same machine groups and visible ordinals.
  • Skip folded groups when numbering, while preserving original indices for drag-and-drop.
  • Document the visible-space numbering and add dependency-free tests against the actual mod with mocked native APIs.

Verification

  • RED: five numbering scenarios failed before the fix; existing agent priority and drag-and-drop behavior passed after correcting the test harness.
  • GREEN: node --test tests/herdr_spaces_test.mjs — 7 passed.
  • python3 scripts/test_refactoria_herdr.py — PASS.
  • python3 scripts/test-refactoria-assets.py — PASS.
  • git diff --check — passed.
  • Native herdr-app check and live UI verification unavailable: Herdr Desktop is not installed in this Linux workspace.

Scope and rollback

Single work unit; revert this commit to restore the previous sidebar numbering, docs, and remove its regression test. No key binding, agent priority, or drag index contract changes.

Risk: none.

@barbatdev

Copy link
Copy Markdown
Owner Author

Fleet review (glm)

Lens: requirements + adversarial. The diff satisfies the acceptance criterion of issue #109: sidebar labels (compact() via ordinal) and Ctrl+A Ctrl+N targets (focusAgent -> spaceOrder()) now derive from the same spaceGroups() computation, so they can no longer desync through grouped()/ordered() reordering. Folded machine groups and unshown spaces occupy no ordinal, the source index i is preserved for drag-and-drop in wide mode, and the early single-group path keeps the headerless rendering identical. The regression tests cover every scenario named in the correction issue (local-vs-vps reorder, worktree created before main, folded group skip and renumber, folded local group only when headers exist, unshown spaces), plus urgency focus and drag-and-drop as no-regression guards. Docs/keymaps.md now describes the visible-space numbering accurately. Two non-blocking notes below. Native UI verification was unavailable to the author (Herdr Desktop not installed in the workspace), which is acceptable given the mocked-native tests exercise the actual mod source.

Suggestion (1)

  • Guard compact() against a null ordinal (herdr/desktop/mods/refactoria-spaces/main.js:454): compact() renders String(ordinal), and spaceGroups() sets ordinal to null exactly for rows of folded groups. Today folded groups render no rows, so compact() never sees a null ordinal, but that invariant lives two functions away in grouped()'s flatMap body. A local guard (e.g. String(ordinal ?? '')) would make the coupling safe if a future edit ever draws a folded group's rows, which would otherwise render the literal string 'null' as the sidebar number.

Question (1)

  • Other consumers of spaceOrder() now skip folded spaces (herdr/desktop/mods/refactoria-spaces/main.js:137): spaceOrder() previously returned every space in display order; it now omits folded groups' spaces and unshown ones. focusAgent() is the intended consumer, but if any other code path (e.g. next/prev space navigation or a 'go to space N' command) also reads spaceOrder(), its behavior silently changes: navigation would jump over folded machines instead of landing on them. Is focusAgent() the only caller of spaceOrder() at this head, and is skipping folded spaces the intended behavior for any other caller?

Verdict: ready to merge

@barbatdev

Copy link
Copy Markdown
Owner Author

Fleet review (deepseek)

Lens: evidence + test design. The fix is correct as far as I can verify: spaceGroups() (herdr/desktop/mods/refactoria-spaces/main.js:123) is now the single source of display order for both the compact labels (compact renders the assigned ordinal, main.js:444) and the shortcut targets (spaceOrder() at main.js:135 flatMaps the same groups, skipping folded ones), and grouped() (main.js:320) consumes the same groups, so the three reported divergences — machine reordering, a worktree created before its main checkout, and folded groups — cannot desynchronise label from target any more. I read the whole mod at the head SHA and confirmed compact has no other caller (only draw at main.js:322), that ...r keeps the source i for drag-and-drop (main.js:425), that groups.length === 1 is equivalent to the old !remote.length early return, and that the claimed scripts/test_refactoria_herdr.py and scripts/test-refactoria-assets.py exist in the tree. The new test file's 7 tests match the 'GREEN: 7 passed' claim and its 5 numbering scenarios match the 'five numbering scenarios' RED claim. No blocking finding: I could not build a concrete input where a rendered number and the space a shortcut focuses differ. Two evidence gaps remain, both in how the new behaviour is protected rather than in the behaviour itself.

Suggestion (2)

  • The new regression test is not run by CI, so the numbering contract it pins can regress unnoticed (.github/workflows/ci.yml): .github/workflows/ci.yml is the repository's only workflow and it enumerates every check as an explicit step (bash tests/gs_test.sh, python3 tests/sync_pi_model_profiles_test.py, python3 scripts/test_pi_appearance.py, ...). Neither the new tests/herdr_spaces_test.mjs nor the two Python suites the PR body cites (scripts/test_refactoria_herdr.py, scripts/test-refactoria-assets.py) appears in it, and the tree has no package.json or task runner that would discover tests/*.mjs. The test therefore only runs when a human remembers the command from the PR body, which is exactly the scenario in which a later refactor of ordered()/spaceGroups() silently re-splits labels from shortcut targets. Evidence: tree at b12ba28 lists tests/herdr_spaces_test.mjs while ci.yml at the same SHA lists no node --test step.
  • Drag-index coverage does not exercise the worktree-nesting reorder the PR claims to preserve (tests/herdr_spaces_test.mjs:94): The PR body claims 'Skip folded groups when numbering, while preserving original indices for drag-and-drop'. Two code paths reorder rows relative to spaces(): machine grouping (exercised by the drag test at tests/herdr_spaces_test.mjs:94, where the rendered order is [local, remote] while the source indices are 1 and 0) and worktree nesting in ordered() (main.js:299-316, exercised only through compact rows by the test at tests/herdr_spaces_test.mjs:58). The combination that the claim is really about — a wide sidebar where a worktree row is emitted under its main checkout but the drag still addresses spaces()[from].id and index: i + 1 (main.js:425) — has no condition in the suite. Missing test condition: wide rows, [{worktree, linked}, {main}] with git as in the existing fixture, mouse-down on the nested worktree row and mouse-up on the main row, expecting the space.move payload to carry the worktree's source index rather than its displayed position.

Verdict: ready to merge

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.

Corrección: feat(herdr): add the missing Herdr TUI keys to Herdr Desktop (#108)

2 participants