Skip to content

feat(herdr): shorten the PR panel title on the collapsed sidebar - #113

Open
barbatdev wants to merge 1 commit into
mainfrom
feat/herdr-collapsed-prs
Open

barbatdev wants to merge 1 commit into
mainfrom
feat/herdr-collapsed-prs

Conversation

@barbatdev

Copy link
Copy Markdown
Owner

Summary

  • On the collapsed sidebar (under 160px, the same rail test as refactoria-chrome), the refactoria-prs header shows a centered PRs label instead of Pull requests plus the repo name, which were cut off.
  • The expanded header is unchanged.

Verification

  • herdr-app check passes.
  • Live: collapsing the sidebar shows PRs; expanding brings back the full title and repo.

🤖 Generated with Claude Code

https://claude.ai/code/session_01A9D7phtHFYfszv6WKeXbiU

On the rail "Pull requests" and the repo name were cut off. The collapsed
sidebar now shows a centered "PRs" label instead.
@barbatdev

Copy link
Copy Markdown
Owner Author

Fleet review (deepseek)

Lens: evidence + test design. The diff is small and does what the PR describes: on the collapsed rail the refactoria-prs header renders a centered PRs label instead of Pull requests plus the repo name, and the expanded header is untouched. No blocking defect found. The open items are evidence questions: the rail test reads the layout tree during build with nothing in the diff subscribing to sidebar width, the sidebar id lookup fails silently into the pre-fix behavior, and the only stated proof of the behavior change is a manual live check with no test asserting the switch. No linked issue was provided, so I reviewed against the PR body's own claims.

Suggestion (3)

  • The layout walk has no cycle guard and runs on every build (herdr/desktop/mods/refactoria-prs/main.js:9): find recurses through children with no visited set or depth bound, and it is invoked on each build of the header. A children chain that references a node already on the stack (self or mutual reference) recurses until the stack overflows instead of returning null. A visited set or a depth cap keeps the fallback behavior bounded.
  • Duplicated header chain and a second hardcoded rail threshold (herdr/desktop/mods/refactoria-prs/main.js:114): Both branches repeat .h(31).items_center().text_size(12).text_color(t.dim), so a later header change has to be applied twice, and 160 is now a magic number duplicated across two mods (refactoria-chrome and refactoria-prs) with no shared source. Extracting the common chain and the threshold into one named constant keeps the rail test from drifting between the mods.
  • Missing test conditions for the label switch (herdr/desktop/mods/refactoria-prs/main.js:19): The only stated proof of the behavior change is herdr-app check passing plus a manual live check; neither asserts the label switch, and the boundary is untested. Test conditions that would evidence the criterion: sidebar size 159px shows PRs; 160px and 161px show Pull requests and the repo name (assert the boundary is on the documented side of < 160); sidebar node absent from the layout tree falls back to the expanded header without throwing; a repo-less space still renders Pull requests alone at wide widths and PRs on the rail.

Question (2)

  • What re-renders the header when the sidebar crosses 160px? (herdr/desktop/mods/refactoria-prs/main.js:19): narrow() calls find(layout(), "sidebar") inline inside the build expression at line 114. Nothing in the diff subscribes to the sidebar width or otherwise marks it as a dependency of this view, so the label can only flip if this View rebuilds for some other reason (space switch, tick, error change). Please state what triggers the rebuild on collapse/expand and whether layout() reads a reactive value in this runtime. The PR body claims a live check showed the label switching, which is why I am asking rather than blocking.
  • sidebar node id and layout export are unverified, and the fallback is silent (herdr/desktop/mods/refactoria-prs/main.js:19): The rail test depends on two assumptions the diff does not show as guaranteed: that layout is an export of herdr for this mod, and that the sidebar node is reachable in the layout tree with the exact id sidebar. If the id is renamed or the node is not yet in the tree during the first build, ?? Infinity makes narrow() false, so the header silently returns to the truncated Pull requests + repo title and the fix appears not to work with no error. Please point at where the sidebar id is defined and note whether the node exists on the first render.

Verdict: ready to merge

@barbatdev

Copy link
Copy Markdown
Owner Author

Fleet review (glm)

Lens: requirements + adversarial. The change swaps the refactoria-prs panel header to a centered PRs label when the sidebar is collapsed (width under 160px), keeping the expanded header unchanged. The find() helper is null/undefined-safe end to end (optional chaining on node?.id, node?.children ?? [], and ?.size?.px ?? Infinity), so a missing layout() tree or a missing sidebar node degrades to the expanded header rather than crashing — a sensible fallback. No linked issue; the PR's stated goal (title/repo cut off on the narrow rail) is directly addressed and the expanded path is byte-identical to the old markup. Two things I could not verify from the diff alone and flag as questions below; neither yields a concrete failure scenario given the PR's claimed live verification (herdr-app check passing and manual collapse/expand observation).

Question (2)

  • How does the header re-render when the sidebar width crosses 160px? (herdr/desktop/mods/refactoria-prs/main.js:15): narrow() reads the layout tree at render time; nothing in the diff subscribes to layout changes. If a sidebar collapse/expand does not trigger a re-render of this view on its own, the label could stay stale until the next poll tick (EVER = 60000ms) or data update. The PR claims live verification that collapsing shows PRs immediately, so this likely works via the framework's layout-driven re-render, but the mechanism is not evident from the diff.
  • Hard-coded node id "sidebar" and the 160px threshold are unverified against refactoria-chrome (herdr/desktop/mods/refactoria-prs/main.js:15): The comment says this is the same rail test as refactoria-chrome (under 160px), and find looks up a node literally named sidebar with a size.px field. If herdr ever renames that layout node id or the chrome mod changes its threshold, this panel silently falls back to the full header. Not a defect today, just a coupling worth confirming both constants actually match the chrome mod.

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.

1 participant