fix(tui): per-route vertical reserve + required panel props (#1069 round-2 follow-up) - #1071
fix(tui): per-route vertical reserve + required panel props (#1069 round-2 follow-up)#1071saravmajestic wants to merge 4 commits into
Conversation
…nd-2 review) Follow-up to the merged #1069, addressing the round-2 review. - Split the vertical reserve per route: HOME_VERTICAL_RESERVE (10) and SESSION_VERTICAL_RESERVE (7) accurately reflect each route's chrome (home has a top spacer + footer; session doesn't), instead of one constant that fit only session. Thresholds re-expressed in usable terms (MEDIUM_MIN_HEIGHT 13, FULL_MIN_HEIGHT 34) so `full` still engages at a ~44-row terminal on home and the classic 80x24 clears the medium floor with margin. - Make WelcomePanel's availableWidth/availableHeight props REQUIRED (drop the raw-terminal fallback + the component's own useTerminalDimensions), so a call site that forgets to scale is a compile error, not a silent regression. - Height now gets the same available-space treatment as width (terminal minus the route's prompt/footer chrome), so a short window shrinks the panel instead of crowding the prompt. - Centralize PANEL_HORIZONTAL_PADDING (4) — was a bare `-4` in the home call site and test helper. Tests: add the sidebar-toggle transition (session 150x50 full -> medium when the sidebar opens — the exact #1067 case), the classic 80x24 on both routes, and keep the isolated-gate / boundary / degenerate coverage. tsgo clean; oxlint 0/0. Reviewed by a 2-model consensus panel (both SOUND); the PANEL_HORIZONTAL_PADDING and sidebar-toggle test came out of that review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
📝 WalkthroughWalkthroughWelcome panel sizing now uses caller-provided available dimensions. Home and session routes apply separate vertical reserves, shared horizontal padding, and session sidebar width. A new workflow dispatches documentation promotion events after documentation changes reach ChangesWelcome Panel Sizing
Documentation Sync Notification
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Home
participant SessionRoute
participant WelcomePanel
participant welcomePanelVariant
Home->>WelcomePanel: provide home available width and height
SessionRoute->>WelcomePanel: provide session available width and height
WelcomePanel->>welcomePanelVariant: calculate responsive variant
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review SummaryStatus: 1 Suggestion Found | Recommendation: Merge (suggestion optional) Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit 2762efb)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 2762efb)Status: 1 Suggestion Found | Recommendation: Merge (suggestion optional) Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (4 files)
Reviewed by glm-5.2 · Input: 47.7K · Output: 16.5K · Cached: 508.8K Review guidance: REVIEW.md from base branch |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/tui/src/component/welcome-panel-utils.ts">
<violation number="1" location="packages/tui/src/component/welcome-panel-utils.ts:37">
P3: The padding centralization is incomplete: PANEL_HORIZONTAL_PADDING is used at the home call site, but the session route's contentWidth still hardcodes the bare `- 4` rather than referencing the constant (the new doc comment even acknowledges session "applies the same 4 as part of its own content-column math"). If this constant ever changes, the session width math won't follow and drifts out of sync; consider having session/index.tsx import PANEL_HORIZONTAL_PADDING so both routes share one source of truth.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // Columns the home slot spends on its own left/right padding (2 + 2); the caller | ||
| // subtracts this to get the panel's usable width. (Session's contentWidth applies | ||
| // the same 4 as part of its own content-column math.) | ||
| export const PANEL_HORIZONTAL_PADDING = 4 |
There was a problem hiding this comment.
P3: The padding centralization is incomplete: PANEL_HORIZONTAL_PADDING is used at the home call site, but the session route's contentWidth still hardcodes the bare - 4 rather than referencing the constant (the new doc comment even acknowledges session "applies the same 4 as part of its own content-column math"). If this constant ever changes, the session width math won't follow and drifts out of sync; consider having session/index.tsx import PANEL_HORIZONTAL_PADDING so both routes share one source of truth.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/tui/src/component/welcome-panel-utils.ts, line 37:
<comment>The padding centralization is incomplete: PANEL_HORIZONTAL_PADDING is used at the home call site, but the session route's contentWidth still hardcodes the bare `- 4` rather than referencing the constant (the new doc comment even acknowledges session "applies the same 4 as part of its own content-column math"). If this constant ever changes, the session width math won't follow and drifts out of sync; consider having session/index.tsx import PANEL_HORIZONTAL_PADDING so both routes share one source of truth.</comment>
<file context>
@@ -7,36 +7,46 @@
+// Columns the home slot spends on its own left/right padding (2 + 2); the caller
+// subtracts this to get the panel's usable width. (Session's contentWidth applies
+// the same 4 as part of its own content-column math.)
+export const PANEL_HORIZONTAL_PADDING = 4
-/** Minimum usable size for the medium panel; below either → compact (one line). */
</file context>
|
@saravmajestic CI is failing on this PR. |
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
Review — round 3
The refactor is the right shape and structurally answers the round-2 comments: per-route reserves are the correct fix, and making the props required is the right way to close the silent-fallback path. But the central quantitative claim — the reserve arithmetic, and the "80×24 clears the medium floor with margin" conclusion drawn from it — doesn't survive checking against the rendered layout.
Major — both vertical reserves undercount the real chrome
welcome-panel-utils.ts:27-32 budgets prompt (~4). Counting the actual Prompt tree at rest (component/prompt/index.tsx:1479-1611):
<box border={["top"]}>— 1 row (top rule, L1479)- inner
paddingTop={1}— 1 row (L1483) <textarea minHeight={1}>— 1 row (L1500)<box height={1} border={["top"]} />— 1 row (separator, L1569)- agent/model meta row — 1 row, always rendered (
fallback={<box height={1} />}, L1572) - status/hint row — 0 or 1. The wrapping
<box>at L1611 is unconditional, but at idle the<Switch>falls through to<Match when={true}>{props.hint ?? <text />}</Match>(L1752), and neither route passeshint— so whether that empty row occupies a line is renderer-dependent.
The prompt is 5-6 rows at rest, not ~4 — 5 are certain, the 6th needs measuring.
Home is undercounted a second time: the reserve omits the home_bottom slot entirely (routes/home.tsx:128). Its built-in provider always returns a box with paddingTop={3} (feature-plugins/home/tips.tsx:34, outside the <Show when={props.show}>), and flexbox shrinks content, not padding — so those 3 rows are unconditional whether or not tips render.
home: top spacer 2 + wrapper 1 + prompt 5-6 + home_bottom 3+ + footer 3 = 14-15+ (constant says 10)
session: gaps 2 + paddingBottom 1 + prompt 5-6 = 8-9 (constant says 7)
Why this matters rather than being a comment nit. The round-2 comment this PR answers was "80×24 sits exactly on the medium floor", and the fix claims the cliff moved to 23. Against MEDIUM_MIN_HEIGHT = 13:
| home reserve | source | home(80,24) available |
variant |
|---|---|---|---|
| 10 | as merged | 14 | medium, margin 1 |
| 11 | 5-row prompt, ignoring home_bottom (most conservative) |
13 | medium, margin 0 — exactly on the floor |
| 12 | 6-row prompt, ignoring home_bottom |
12 | compact |
| 14-15 | prompt + home_bottom |
9-10 | compact |
Every corrected reading destroys the claimed margin. The most conservative one puts 80×24 back exactly on the medium floor — verbatim the round-2 complaint — and the rest drop it to compact. The margin exists only at reserve 10, and expect(home(80,24)).toBe("medium") is pinned to that constant rather than to the rendered layout.
To be precise about the consequence: at 80×24 the unconnected medium panel plus a one-line tip fits almost exactly, so this isn't guaranteed overflow — it clips once a tip wraps to a second line. The defect is the absent margin, not certain breakage.
Suggested fix — derive the reserves from the real tree and re-tune the thresholds separately, so the two stop compensating for each other:
// Measure once against a real render and fix the value; 5 is the floor,
// 6 if the idle hint row occupies a line.
const PROMPT_REST_HEIGHT = 6
export const HOME_VERTICAL_RESERVE =
2 + // top spacer (home.tsx:109)
1 + // prompt wrapper paddingTop (home.tsx:123)
PROMPT_REST_HEIGHT +
3 + // home_bottom paddingTop (tips.tsx:34)
3 // footer (feature-plugins/home/footer.tsx:64-82)
// 15
export const SESSION_VERTICAL_RESERVE = 2 + 1 + PROMPT_REST_HEIGHT // 9Then choose MEDIUM_MIN_HEIGHT / FULL_MIN_HEIGHT against the panel's actual heights (medium ≈ 6-8 rows, full ≈ 13) plus whatever product breakpoints you want. If 80×24 must stay medium, that becomes an explicit threshold decision instead of a hidden one.
Minor
1. PANEL_HORIZONTAL_PADDING isn't actually centralized — welcome-panel-utils.ts:34-37, routes/session/index.tsx:275
The constant's comment says session "applies the same 4 as part of its own content-column math", but contentWidth is still dimensions().width - (sidebarVisible() ? 42 : 0) - 4 — literal 4, literal 42, neither imported. Only home.tsx uses the constant. Either import it at session/index.tsx:275 (and add a SIDEBAR_WIDTH alongside), or drop the claim from the comment.
2. Sidebar width 42 is duplicated between production and the test helper — test/component/welcome-panel-utils.test.ts:22, routes/session/index.tsx:275
If the sidebar width changes, production and tests drift silently in the same direction that produced #1067.
3. Test helpers re-implement the call-site arithmetic instead of exercising it — test/component/welcome-panel-utils.test.ts:19-22
home() and session() duplicate exactly what home.tsx:116-117 and session/index.tsx:1194-1195 compute, so the "verified terminal → variant" table proves a property of the helpers, not of the routes — which is precisely why the Major finding above passed CI green. Required props catch a missing prop, never a wrong one, so they don't cover this. Cheap fix short of the deferred render test: export homeAvailable(w,h) / sessionAvailable(w,h,sidebar) from the utils module and call them from both the routes and the tests, so there's one definition of the arithmetic.
4. Worth confirming intent: session sizes against the sidebar even when it's an overlay — routes/session/index.tsx:267, 275, 1194
contentWidth subtracts 42 whenever sidebarVisible(), but the sidebar only consumes column width when wide() (width > 120). Below that it renders position="absolute" (session/index.tsx:1360-1371) and takes no layout width, so on a 100-col terminal with the sidebar opened contentWidth() reports 54 while the panel has ~96 columns of layout space, and the panel drops to compact.
Raising this as a question, not a defect — the overlay does visually cover roughly those columns behind a dimmed backdrop, so sizing to the obscured width may well be deliberate, and it can't produce the opposite (wrongly-full) error since full needs ≥156 raw cols where wide() is always true. Worth a line of comment recording which behaviour is intended, now that panel sizing routes through contentWidth.
Nit
5. The height thresholds read as fit requirements but aren't — welcome-panel-utils.ts:39-44
MEDIUM_MIN_HEIGHT = 13 / FULL_MIN_HEIGHT = 34 are documented as "minimum usable size for the … panel", but the medium panel is ~6-8 rows and the full panel ~13 (welcome-panel.tsx:88-138). They're breathing-room heuristics several times the actual need, and nothing records why 13 and 34 — relevant once the Major finding forces a re-tune.
What's done well
- Splitting
PANEL_VERTICAL_RESERVEper route is the right response to the round-2 finding, and the comments walk the JSX rather than asserting a number. The home footer (3 rows: paddingTop 1 + content 1 + paddingBottom 1) and the<box height={2}>spacer are both counted correctly, as is the session gap count — 3 in-flow children withgap={1}gives exactly 2 gaps, sinceToastisposition="absolute"and out of flow. - Making the props required and deleting the component's internal
useTerminalDimensionsremoves the silent-fallback path that would have reintroduced #1067. Both call sites are in-repo, so there's no external breakage. - The added sidebar-toggle test (
session(150,50,false) → fullvssession(150,50,true) → medium) pins the actual reported bug rather than a proxy — the most valuable test in the file. - Gate isolation is real:
TALL/WIDEderive from the constants, so each test fails if its own<is deleted or flipped to<=. - The reactivity note at
welcome-panel.tsx:40-41is correct — readingprops.xinsidecreateMemotracks, so the variant recomputes on resize and sidebar toggle.
Missing tests
- A render test of either call site — the acknowledged gap, and the one that would have caught the Major finding and Minor 3. Given this is the second round on the same bug, I'd treat it as the remaining open item rather than a follow-up.
- A test tying the reserves to the real chrome (
HOME_VERTICAL_RESERVE === SPACER + WRAPPER + PROMPT_REST_HEIGHT + HOME_BOTTOM + FOOTERfrom named constants), which would make the Major finding impossible to reintroduce silently. - The overlay-sidebar case from Minor 4: sidebar open on a ≤120-col terminal.
- Negative available dimensions (an 80×5 terminal yields height
-5on home); only(0,0)and(1,1)are covered today.
One caveat on the numbers above: I read the layout tree rather than running it, so PROMPT_REST_HEIGHT is 5-or-6 pending one measurement against a real render. The conclusion holds either way — even the most conservative count puts 80×24 back on the floor.
…sts) - correct per-route vertical reserves to the real chrome: HOME 10->15, SESSION 7->9, derived from PROMPT_REST_HEIGHT=6 plus the counted home_bottom slot and footer (the prompt is 5-6 rows at rest, not ~4) - retune height thresholds honestly: MEDIUM_MIN_HEIGHT 13->8 (the real medium panel height, so 80x24 is a genuine medium fit, not a pinned margin), FULL_MIN_HEIGHT 34->30 (a documented product breakpoint, not a fit minimum) - centralize the route arithmetic: export homeAvailable/sessionAvailable and call them from both the routes and the tests, so a wrong reserve/threshold fails a test instead of being mirrored in a test-only copy - centralize PANEL_HORIZONTAL_PADDING and add SIDEBAR_WIDTH (drop the duplicated 42) - keep the panel on session's contentWidth basis (narrows with the sidebar, incl. the dimmed overlay) so the panel stays aligned with the messages; documented - tests: reserve-pinned-to-chrome, overlay narrowing, negative dimensions Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Mounts the real WelcomePanel through the app context stack at fixed available sizes and asserts each variant renders its distinct content (full: "What is Altimate Code"; medium: the condensed line, no what-is section; compact: the single connect-CTA line). Covers the .tsx Switch that the pure welcomePanelVariant unit test can't. Deliberately not a full Home/Session route render — that drags in Prompt's ~28-context dependency graph; the route wiring is now a shared one-liner through homeAvailable/sessionAvailable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the round-3 pass — the reserve-arithmetic catch was exactly right. All items addressed in Major — reserves undercount the real chromeFixed. Reserves are now derived from the counted tree, not estimated:
And the thresholds are re-tuned separately so they stop compensating for each other:
Minor
NitThe height thresholds now read for what they are: Missing tests
CIThe red "TypeScript" job is a pre-existing unrelated flake — the only failing test was These changes were also cross-checked with a two-model consensus review (both APPROVE). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/tui/test/component/welcome-panel.test.tsx`:
- Around line 26-74: Update the tests using renderPanel so each test owns its
returned app and wraps execution in a try/finally block that destroys
app.renderer and resets the onboarding state. Remove the module-level current
variable and its afterEach cleanup, ensuring cleanup still runs when assertions
or rendering fail and no renderer can be overwritten across concurrent tests.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0367c3cc-e91a-4591-8794-553241bae06a
📒 Files selected for processing (5)
packages/tui/src/component/welcome-panel-utils.tspackages/tui/src/routes/home.tsxpackages/tui/src/routes/session/index.tsxpackages/tui/test/component/welcome-panel-utils.test.tspackages/tui/test/component/welcome-panel.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/tui/test/component/welcome-panel-utils.test.ts
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| // terminals) so the panel stays aligned with the messages and both restore | ||
| // together when it closes. Height reserves the prompt row. Shared arithmetic in | ||
| // welcome-panel-utils so the tests exercise this exact math. | ||
| const panelAvailable = createMemo(() => sessionAvailable(dimensions().width, dimensions().height, sidebarVisible())) |
There was a problem hiding this comment.
SUGGESTION: panelAvailable().width duplicates the contentWidth() formula
The note above states panelAvailable shares contentWidth's content-column basis, and sessionAvailable(...).width computes the exact same value as the contentWidth memo (line 275): width - (sidebarVisible ? SIDEBAR_WIDTH : 0) - PANEL_HORIZONTAL_PADDING. Since sessionAvailable is already the unit-tested single source, consider deriving contentWidth from it (declare panelAvailable first, then const contentWidth = createMemo(() => panelAvailable().width)) so the width math has one definition and can't structurally drift.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/tui/test/component/welcome-panel.test.tsx">
<violation number="1" location="packages/tui/test/component/welcome-panel.test.tsx:70">
P3: The renderer created inside `renderPanel()` is only registered for cleanup after the function returns (via `current = app` in each caller). If rendering or capture throws before that assignment, the leaked app's renderer/event listeners survive — `afterEach` still points at the previous test's already-destroyed app. Make cleanup robust by assigning `current` inside `renderPanel()` (or using try/finally around `renderOnce`) so a failing render still destroys its resources.</violation>
</file>
<file name="packages/tui/src/component/welcome-panel-utils.ts">
<violation number="1" location="packages/tui/src/component/welcome-panel-utils.ts:97">
P2: On a narrow session with the sidebar toggled open, the welcome panel is sized as though 42 columns were removed even though the overlay leaves its layout box full width, so medium content can be laid out across the obscured area and appear truncated. The available-width calculation should distinguish the in-flow (`wide()`) sidebar from the absolute overlay, or the panel wrapper should be constrained to the narrowed width.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| sidebarVisible: boolean, | ||
| ): { width: number; height: number } { | ||
| return { | ||
| width: terminalWidth - (sidebarVisible ? SIDEBAR_WIDTH : 0) - PANEL_HORIZONTAL_PADDING, |
There was a problem hiding this comment.
P2: On a narrow session with the sidebar toggled open, the welcome panel is sized as though 42 columns were removed even though the overlay leaves its layout box full width, so medium content can be laid out across the obscured area and appear truncated. The available-width calculation should distinguish the in-flow (wide()) sidebar from the absolute overlay, or the panel wrapper should be constrained to the narrowed width.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/tui/src/component/welcome-panel-utils.ts, line 97:
<comment>On a narrow session with the sidebar toggled open, the welcome panel is sized as though 42 columns were removed even though the overlay leaves its layout box full width, so medium content can be laid out across the obscured area and appear truncated. The available-width calculation should distinguish the in-flow (`wide()`) sidebar from the absolute overlay, or the panel wrapper should be constrained to the narrowed width.</comment>
<file context>
@@ -54,3 +69,32 @@ export function welcomePanelVariant(width: number, height: number): WelcomePanel
+ sidebarVisible: boolean,
+): { width: number; height: number } {
+ return {
+ width: terminalWidth - (sidebarVisible ? SIDEBAR_WIDTH : 0) - PANEL_HORIZONTAL_PADDING,
+ height: terminalHeight - SESSION_VERTICAL_RESERVE,
+ }
</file context>
| return { app, frame } | ||
| } | ||
|
|
||
| let current: Awaited<ReturnType<typeof testRender>> | undefined |
There was a problem hiding this comment.
P3: The renderer created inside renderPanel() is only registered for cleanup after the function returns (via current = app in each caller). If rendering or capture throws before that assignment, the leaked app's renderer/event listeners survive — afterEach still points at the previous test's already-destroyed app. Make cleanup robust by assigning current inside renderPanel() (or using try/finally around renderOnce) so a failing render still destroys its resources.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/tui/test/component/welcome-panel.test.tsx, line 70:
<comment>The renderer created inside `renderPanel()` is only registered for cleanup after the function returns (via `current = app` in each caller). If rendering or capture throws before that assignment, the leaked app's renderer/event listeners survive — `afterEach` still points at the previous test's already-destroyed app. Make cleanup robust by assigning `current` inside `renderPanel()` (or using try/finally around `renderOnce`) so a failing render still destroys its resources.</comment>
<file context>
@@ -0,0 +1,97 @@
+ return { app, frame }
+}
+
+let current: Awaited<ReturnType<typeof testRender>> | undefined
+afterEach(() => {
+ current?.renderer.destroy()
</file context>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/notify-help-docs.yml:
- Line 25: Update the runs-on value used by the notify-help-docs workflow so
actionlint recognizes it: either register arc-runner-gke under runner-label in
actionlint.yaml if it is the intended self-hosted label, or replace it with an
existing configured runner label. Preserve the workflow’s intended runner
behavior.
- Around line 54-60: Update the create-github-app-token step identified by id
tok to request only repository contents write access, using the action’s
permissions configuration while preserving the existing app ID, private key,
owner, and repository settings.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7406cdbc-a2f9-42ea-960a-502786f6f7bb
📒 Files selected for processing (4)
.github/workflows/notify-help-docs.ymlpackages/tui/src/routes/home.tsxpackages/tui/src/routes/session/index.tsxpackages/tui/test/component/welcome-panel.test.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/tui/test/component/welcome-panel.test.tsx
- packages/tui/src/routes/home.tsx
- packages/tui/src/routes/session/index.tsx
| jobs: | ||
| notify: | ||
| if: ${{ !contains(github.event.head_commit.message, '[docs-sync]') }} | ||
| runs-on: arc-runner-gke |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -HI -t f 'actionlint\.ya?ml' . -x sh -c '
echo "=== $1 ==="
cat "$1"
' sh
actionlint .github/workflows/notify-help-docs.ymlRepository: AltimateAI/altimate-code
Length of output: 1056
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== workflow runs-on lines ==="
rg -n "runs-on:" .github/workflows || true
echo "=== actionlint config files ==="
fd -HI -t f 'actionlint\.ya?ml' . || true
echo "=== actionlint versions/config references ==="
rg -n "runner-label|arc-runner-gke|actionlint" -S .github actions . || trueRepository: AltimateAI/altimate-code
Length of output: 1848
Register arc-runner-gke with actionlint.
arc-runner-gke is unknown to actionlint and causes notify-help-docs.yml to fail linting. Add it to actionlint.yaml under runner-label if this is a self-hosted runner label, or switch this workflow to an already configured runner label.
🧰 Tools
🪛 actionlint (1.7.12)
[error] 25-25: label "arc-runner-gke" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2025-vs2026", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xlarge", "macos-latest-large", "macos-26-intel", "macos-26-xlarge", "macos-26-large", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xlarge", "macos-14-large", "macos-14", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/notify-help-docs.yml at line 25, Update the runs-on value
used by the notify-help-docs workflow so actionlint recognizes it: either
register arc-runner-gke under runner-label in actionlint.yaml if it is the
intended self-hosted label, or replace it with an existing configured runner
label. Preserve the workflow’s intended runner behavior.
Source: Linters/SAST tools
| - uses: actions/create-github-app-token@d72941d797fd3113feb6b93fd0dec494b13a2547 # v1 | ||
| id: tok | ||
| with: | ||
| app-id: ${{ vars.DOCS_SYNC_APP_ID }} | ||
| private-key: ${{ secrets.DOCS_SYNC_APP_PRIVATE_KEY }} | ||
| owner: AltimateAI | ||
| repositories: help-docs |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow excerpt =="
sed -n '1,140p' .github/workflows/notify-help-docs.yml
echo
echo "== action metadata/usage references =="
for f in .github/workflows/notify-help-docs.yml; do
echo "-- $f"
rg -n "create-github-app-token|permission-contents|contents:|write|permissions:" "$f" || true
done
echo
echo "== git diff stat/name-status =="
git diff --stat || true
git status --short || trueRepository: AltimateAI/altimate-code
Length of output: 2944
🌐 Web query:
actions/create-github-app-token permission-contents default behavior repository permissions v1
💡 Result:
In the actions/create-github-app-token GitHub Action, the permission-contents input allows you to specify the access level for repository contents (such as commits and branches) granted to the generated installation access token [1][2][3]. The default behavior for the permission-contents input (and all other permission-* inputs) when left unspecified is that the generated token inherits all permissions granted to the GitHub App installation [1][4][5]. Key points regarding this behavior include: 1. Inherited Scope: If you do not explicitly define a permission-contents value, the resulting token will possess all permissions that were configured for the app installation on the repository or account [1][6][7]. 2. Best Practices: It is highly recommended to explicitly define the permissions required for your specific use case (e.g., setting permission-contents: write) to follow the principle of least privilege [1][8][3]. 3. Constraints: You cannot grant the token permissions that the GitHub App installation itself does not already possess [9][8][6]. Attempting to request a permission via the action that the installation lacks will result in an error [8][6]. 4. Functional Scope: While earlier versions of the action had reports of the permission-* inputs not correctly restricting the token in some scenarios [10][9], modern versions correctly use these inputs to restrict the token scope when interacting with the GitHub API's installation access token endpoint [9]. If you are using version 1 of the action, be aware that while the fundamental logic of requesting a token remains consistent, newer versions (v2+) include improved handling and enforcement of these granular permission inputs [10].
Citations:
- 1: https://github.com/actions/create-github-app-token/blob/main/README.md
- 2: https://github.com/actions/create-github-app-token/blob/29824e69f54612133e76f7eaac726eef6c875baf/action.yml
- 3: https://deepwiki.com/actions/create-github-app-token/4.2-permission-system
- 4: https://github.com/actions/create-github-app-token
- 5: https://github.com/actions/create-github-app-token/blob/34c66235f6ccc38eb01b7aed860890bdf40db33c/README.md
- 6: https://deepwiki.com/actions/create-github-app-token/4.1-action-inputs
- 7: https://github.com/actions/create-github-app-token/tree/v1.9.2
- 8: https://github.com/marketplace/actions/create-github-app-token
- 9: Using
permission-*option withactions/create-github-app-tokendoes not restrict token permissions actions/create-github-app-token#237 - 10: Changes in permission-* handling since version 2.0.4 actions/create-github-app-token#248
🌐 Web query:
actions/create-github-app-token permission-contents default unspecified action.yml
💡 Result:
In the actions/create-github-app-token action, the permission-contents input (and other similar permission-* inputs) does not have a default value specified in the action.yml file [1][2]. When these permission-* inputs are left unspecified by the user in their workflow YAML: 1. The action does not apply any specific restrictions or overrides to the generated token's scopes [3][4]. 2. By default, the generated GitHub App installation token inherits all permissions currently granted to the GitHub App's installation [5][6][7]. The permission-* inputs are provided as optional parameters to allow users to implement the principle of least privilege by explicitly restricting the token's permissions to only those necessary for a specific job [5][3]. If you choose to use these inputs, you must specify them (e.g., permission-contents: read or permission-contents: write) [1][3]. If no such inputs are provided, the action defaults to the full set of permissions defined on the app installation itself [6][4]. Note that if you explicitly set any permission-* input, you are defining the exact scope for the generated token; if the specified permission is not granted to the app installation, the request will fail [5][6][4].
Citations:
- 1: https://github.com/actions/create-github-app-token/blob/main/action.yml
- 2: https://github.com/actions/create-github-app-token/blob/df432cee/action.yml
- 3: https://deepwiki.com/actions/create-github-app-token/4.2-permission-system
- 4: https://deepwiki.com/actions/create-github-app-token/4.1-action-inputs
- 5: https://github.com/actions/create-github-app-token/blob/5c1e00d1af39c515564da2ae21cc9dedf0793346/README.md
- 6: https://github.com/actions/create-github-app-token
- 7: https://github.com/marketplace/actions/create-github-app-token
🌐 Web query:
"actions/create-github-app-token" "In this case" "permission" "default to" "installation"
💡 Result:
When using the actions/create-github-app-token action, the token's permissions are determined by how you configure the action's inputs [1][2]. By default, if you do not specify any permission-related inputs, the generated token inherits all of the permissions granted to the GitHub App's installation [1][3][4]. However, if you explicitly provide any permission-related inputs (e.g., permission-contents: read), the token's permissions are restricted to only those explicitly defined [4]. In this scenario, the token does not inherit the full set of installation permissions, but is instead scoped specifically to the inputs provided [4]. Key points regarding permissions: - Default Behavior: Inherits all installation permissions [1][3]. - Explicit Configuration: Providing any permission-specific input (prefixed with permission-) overrides the default, and the token is scoped only to those provided permissions [4]. - Requirement: The permissions you request (either by default or explicitly) must be granted to the GitHub App installation itself; otherwise, the action will fail with an error [1][3]. - Minimum Requirement: The GitHub App generally requires at least "Repository Metadata: read" permissions to successfully locate the installation and mint the token [5].
Citations:
- 1: https://github.com/actions/create-github-app-token
- 2: https://github.com/marketplace/actions/create-github-app-token
- 3: https://github.com/actions/create-github-app-token/blob/5c1e00d1af39c515564da2ae21cc9dedf0793346/README.md
- 4: fix: restore permission-discussions in GitHub App token fields github/gh-aw#25709
- 5: What is the minimum required permissions for GitHub App actions/create-github-app-token#74
Restrict the GitHub App token permissions.
This token inherits every permission granted to the App installation on AltimateAI/help-docs. The dispatch needs only repository contents write access. Mint the app token with only that permission to reduce the impact of token exposure.
Proposed change
- uses: actions/create-github-app-token@d72941d797fd3113feb6b93fd0dec494b13a2547 # v1
id: tok
with:
app-id: ${{ vars.DOCS_SYNC_APP_ID }}
private-key: ${{ secrets.DOCS_SYNC_APP_PRIVATE_KEY }}
owner: AltimateAI
repositories: help-docs
+ permission-contents: write📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/create-github-app-token@d72941d797fd3113feb6b93fd0dec494b13a2547 # v1 | |
| id: tok | |
| with: | |
| app-id: ${{ vars.DOCS_SYNC_APP_ID }} | |
| private-key: ${{ secrets.DOCS_SYNC_APP_PRIVATE_KEY }} | |
| owner: AltimateAI | |
| repositories: help-docs | |
| - uses: actions/create-github-app-token@d72941d797fd3113feb6b93fd0dec494b13a2547 # v1 | |
| id: tok | |
| with: | |
| app-id: ${{ vars.DOCS_SYNC_APP_ID }} | |
| private-key: ${{ secrets.DOCS_SYNC_APP_PRIVATE_KEY }} | |
| owner: AltimateAI | |
| repositories: help-docs | |
| permission-contents: write |
🧰 Tools
🪛 zizmor (1.29.0)
[error] 54-54: dangerous use of GitHub App tokens (github-app): app token inherits blanket installation permissions
(github-app)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/notify-help-docs.yml around lines 54 - 60, Update the
create-github-app-token step identified by id tok to request only repository
contents write access, using the action’s permissions configuration while
preserving the existing app ID, private key, owner, and repository settings.
Source: Linters/SAST tools
…t renderer isolation; derive contentWidth from panelAvailable - Marker Guard: home.tsx panelAvailable and session contentWidth/panelAvailable are edits in upstream-shared files, now wrapped in altimate_change markers - derive contentWidth from panelAvailable().width — one definition, can't drift - render test: own the renderer per test (destroy + reset in finally) instead of a shared module-level var, safe under parallel bun test Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| check(frame) | ||
| } finally { | ||
| app.renderer.destroy() | ||
| resetSetupComplete() |
There was a problem hiding this comment.
SUGGESTION: resetSetupComplete() in finally is redundant
renderPanel already calls resetSetupComplete() at its start (line 27) before every render, so each test begins with onboarding state deterministically reset regardless of what the previous test left behind. The finally reset here has no observable effect on any later test — app.renderer.destroy() is the only load-bearing cleanup in this block.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
0757b82 to
a5e1b50
Compare
2 participants
Follow-up to the merged #1069, addressing @sahrizvi's round-2 review. Also passed a 2-model consensus panel (Claude + Gemini) — both SOUND.
What changed (from the round-2 comments)
MINOR — reserve fit only session, under-reserved home. Split into per-route constants that match each route's actual chrome:
HOME_VERTICAL_RESERVE = 10(top spacer 2 + prompt wrapper 1 + prompt ~4 + footer ~3)SESSION_VERTICAL_RESERVE = 7(gaps 2 + paddingBottom 1 + prompt ~4; no top spacer/footer in that column)Thresholds re-expressed in usable terms (
MEDIUM_MIN_HEIGHT 13,FULL_MIN_HEIGHT 34) sofullstill engages at a ~44-row terminal on home, and the classic 80×24 clears the medium floor with margin (the cliff the review flagged now sits at 23, off the common size).MINOR — optional props silently defaulted to raw terminal. Made
availableWidth/availableHeightrequired and dropped the component's ownuseTerminalDimensions+ fallback. A call site that forgets to scale is now a compile error, not a silent regression — this is the type-level guard for the "no render test" finding.NITs
fullengages ~3 rows sooner on session (≥41) than home (≥44) — intentional (session has less chrome, so more usable height).From the consensus panel (both models endorsed)
PANEL_HORIZONTAL_PADDING = 4— was a bare-4in the home call site + test helper (vertical reserves were already named constants).session(150,50,false) → fullvssession(150,50,true) → medium— pins the exact home_logo slot always visible and cannot be dismissed in TUI #1067 case directly.Effective terminal → variant (verified)
Verification
tsgoclean · 12 unit + 11 render/adjacent tests pass · oxlint 0/0 · prettier. Gate-isolation tests still kill mutants (deleting or<→<=-flipping any gate fails a test).Deferred (noted by both reviewers): a session-with-sidebar render test as the true call-site regression guard — happy to add as a follow-up.
🤖 Generated with Claude Code
Summary by cubic
Fixes welcome panel sizing so variants are chosen by available space per route, not raw terminal size. Updates reserves, thresholds, and shared route math to prevent “full” in the session sidebar and on short terminals.
Bug Fixes
HOME_VERTICAL_RESERVE = 15,SESSION_VERTICAL_RESERVE = 9.MEDIUM_MIN_HEIGHT = 8,FULL_MIN_HEIGHT = 30. Results: 80×24 → medium; home full at ≥45 rows; session full at ≥39; 120×22 → compact.homeAvailable/sessionAvailable, sharedPANEL_HORIZONTAL_PADDING = 4andSIDEBAR_WIDTH = 42; sessioncontentWidthderives frompanelAvailable().width.Migration
WelcomePanelnow requiresavailableWidthandavailableHeight. Pass values fromhomeAvailable(...)/sessionAvailable(...).Written for commit a5e1b50. Summary will update on new commits.
Summary by CodeRabbit