fix(desktop): indent project session rows - #3175

Merged
Astro-Han merged 2 commits into
apache:mainfrom
ARE404:codex/fix-sidebar-session-indentation
Aug 18, 2026
Merged

fix(desktop): indent project session rows#3175
Astro-Han merged 2 commits into
apache:mainfrom
ARE404:codex/fix-sidebar-session-indentation

Conversation

@ARE404

@ARE404ARE404 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Restore the standard spacing-6 (24px) SideNav nesting step for sessions rendered under a project. The CSS contract test pins the hierarchy, and the Electron test verifies the visible indentation after waiting for both measured controls to become visible.

Verification

  • npx biome check apps/desktop/src/renderer/styles/sidebar.css packages/ui/src/session-history-list.tsx apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts apps/desktop/e2e/sidebar-project-row.spec.ts — passed
  • node --test apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts — 1 passed
  • npx playwright test --config e2e/playwright.config.ts e2e/sidebar-project-row.spec.ts — 3 passed
  • Desktop build and focused compiled contract tests passed before review

Review focus

The implementation restores the existing design-system spacing token rather than introducing a new value. The geometry assertion waits for the project and first-session controls to be visible before reading boundingBox(), addressing the review reliability concern.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex audited the sidebar hierarchy, implemented and tested the CSS/E2E fix, incorporated review feedback, and drafted this description. The human contributor reviewed the work, chose to submit it, and remains responsible for its accuracy, provenance, and licensing.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@coderabbitai

coderabbitaiBot commented Aug 17, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 93f15545-8c06-4318-bc6b-27b2be7308e2

📥 Commits

Reviewing files that changed from the base of the PR and between 5d9ce0d and e1906de.

📒 Files selected for processing (4)
  • apps/desktop/e2e/sidebar-project-row.spec.ts
  • apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts
  • apps/desktop/src/renderer/styles/sidebar.css
  • packages/ui/src/session-history-list.tsx

Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.


📝 Walkthrough

What this PR solves

Project session rows in the desktop sidebar lost their normal nesting indentation because the child container applied 0 !important. This PR restores the standard spacing-6 24px indentation. It also preserves selected-state inset and right-edge alignment.

Source of truth

The PR extends the existing sidebar CSS behavior. It does not create a parallel rendering path or public API. The contract test reads sidebar.css and verifies the required hierarchy rule.

Solution scope and complexity

The change is the smallest coherent solution:

  • Remove the 0 !important indentation override.
  • Add the required --spacing-6 offset.
  • Update the related component comment.
  • Add focused CSS contract and Electron geometry coverage.

The added tests are necessary to prevent both stylesheet regressions and visible layout regressions. No added implementation complexity appears unnecessary.

Simplification opportunities

No code or test can be removed without weakening either the CSS contract or the user-visible geometry coverage. The test setup could only be simplified if another existing test already provided equivalent coverage for both the rule and rendered spacing.

Risks and validation

The change affects desktop sidebar layout. Session rows now appear 24px farther from the project row. Incorrect CSS specificity could still prevent the intended rule from applying.

The PR adds:

  • A CSS contract test for the explicit 24px hierarchy rule.
  • An Electron test that waits for sidebar geometry and verifies at least 20px of horizontal separation.
  • Existing validation reported by the PR: desktop builds, unit tests, Playwright tests, Biome checks, and layout detection.

The final status of required checks is unverified from the available evidence.

Review-relevant risks

The current diff has a user-visible desktop layout effect. Material changes in user-visible behavior require independent human review under repository policy.

No public contract, security, licensing, release, or governance effect was identified in the current diff.

The person performing the merge must review the final diff. A maintainer makes the final determination.

Walkthrough

Project child session rows now render with a 24px nesting offset. Contract and end-to-end tests verify the stylesheet rule and horizontal separation.

Changes

Sidebar hierarchy

Layer / File(s)Summary
Project session indentation and validation
apps/desktop/src/renderer/styles/sidebar.css, packages/ui/src/session-history-list.tsx, apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts, apps/desktop/e2e/sidebar-project-row.spec.ts
Project child sessions use var(--spacing-6) inline-start padding. The related comment describes the nesting. Contract tests verify the CSS rule, and end-to-end tests verify visible horizontal separation.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk:⚪ Minimal · up to e1906

This PR restores project/session indentation and adds targeted regression coverage; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers:astro-han, jackwener

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Ai Use Disclosure⚠️ WarningThe PR description selects neither AI-use declaration, and both introduced commits have no Generated-by trailer.Select exactly one declaration and, if applicable, name the tool and scope. Follow CONTRIBUTING.md “Human ownership and AI attribution”; retain required trailers through squash or amend.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.
Description check✅ PassedThe description includes the required summary, verification, AI use, and checklist sections with specific implementation details and test results.
Title check✅ PassedThe title clearly and concisely describes the main change: restoring indentation for project session rows.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@ARE404
ARE404 marked this pull request as ready for review August 17, 2026 16:11
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix desktop sidebar: indent project session rows (restore spacing-6)

🐞 Bug fix🧪 Tests🕐 20-40 Minutes

Grey Divider

AI Description

• Restore Astryx SideNav child indentation (spacing-6 / 24px) for sessions under projects.
• Add a CSS contract test to pin the project→session hierarchy rule.
• Strengthen the Electron sidebar Playwright test with a geometry-based indentation assertion.
Diagram

graph TD
A["ProjectNavRow (ui)"] --> B["Electron renderer"] --> C["sidebar.css rule"]
D["CSS contract test"] --> C
E["Playwright sidebar e2e"] --> B
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Move indentation control into the component (StyleX)
  • ➕ Eliminates reliance on unlayered CSS overrides + !important
  • ➕ Keeps hierarchy styling co-located with the rendering logic
  • ➖ May require StyleX layering/ordering changes that are broader than this fix
  • ➖ Harder to validate across build outputs if CSS extraction differs by environment
2. Rely on Playwright geometry test only (drop CSS contract test)
  • ➕ Less brittle than regex-matching a CSS selector/body
  • ➕ Ensures the user-visible outcome is correct
  • ➖ Slower and potentially flakier than a pure unit/contract test
  • ➖ Harder to pinpoint regressions (CSS rule vs layout/DOM change)

Recommendation: Current approach (restore the design-system spacing token in sidebar.css, plus a fast CSS contract test and a user-visible geometry assertion) is the best tradeoff: it fixes the regression with minimal surface area, locks in the intended hierarchy at two levels (source CSS and rendered layout), and avoids introducing new spacing values.

Files changed (4) +44 / -8

Bug fix (1) +8 / -7
sidebar.cssRestore spacing-6 indentation for project child session container+8/-7

Restore spacing-6 indentation for project child session container

• Updates the .maka-project-row child-group rule to apply padding-inline-start: var(--spacing-6) !important instead of zeroing indentation. Rewrites the comment to reflect that project grouping is a true parent-child hierarchy and why the value is pinned.

apps/desktop/src/renderer/styles/sidebar.css

Tests (2) +35 / -0
sidebar-project-row.spec.tsAdd geometry assertion for project→session indentation+8/-0

Add geometry assertion for project→session indentation

• Captures bounding boxes for the project navigation control and the first session control, asserting the session is horizontally offset. This makes the indentation regression detectable in the Electron E2E suite rather than relying only on focus/order assertions.

apps/desktop/e2e/sidebar-project-row.spec.ts

session-project-hierarchy-contract.test.tsAdd contract test pinning sidebar CSS hierarchy rule+27/-0

Add contract test pinning sidebar CSS hierarchy rule

• Introduces a Node test that locates sidebar.css across source/dist paths and asserts the presence of a specific project-children selector. Pins the required padding-inline-start to var(--spacing-6) with !important as a visual contract.

apps/desktop/src/main/tests/session-project-hierarchy-contract.test.ts

Documentation (1) +1 / -1
session-history-list.tsxUpdate ProjectNavRow comment to match indentation behavior+1/-1

Update ProjectNavRow comment to match indentation behavior

• Adjusts the inline comment above the nested session rendering to reflect that sidebar.css preserves a standard SideNav nesting step for the project hierarchy.

packages/ui/src/session-history-list.tsx

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1)📘 Rule violations (0)📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Geometry assertion races visibility 🐞 Bug☼ Reliability
Description
The new test samples navigation.boundingBox() and firstSessionControl.boundingBox() immediately
after switching to project mode, without waiting for either element to be visible first.
boundingBox() does not auto-wait for visibility like assertion matchers do, so it can return
null while the sidebar is still re-rendering, making the new non-null assertions intermittently
fail in CI.
Code

apps/desktop/e2e/sidebar-project-row.spec.ts[R37-43]

+ const [projectNavigationBox, firstSessionBox] = await Promise.all([+ navigation.boundingBox(),+ firstSessionControl.boundingBox(),+ ]);+ expect(projectNavigationBox).not.toBeNull();+ expect(firstSessionBox).not.toBeNull();+ expect(firstSessionBox!.x - projectNavigationBox!.x).toBeGreaterThanOrEqual(20);
Relevance

●●● Strong

Matching accepted precedent explicitly requires visibility waits before boundingBox after UI
transitions.

PR-#3160

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The project-mode radio is clicked at line 20 and the DOM updates asynchronously
(aria-expanded/aria-controls wiring, session subtree mount). The new code at lines 37-43 immediately
calls boundingBox() on both locators and asserts non-null without first waiting for visibility
(e.g., via toBeVisible()), which is a documented gap since boundingBox() returns null for elements
not currently rendered/visible instead of waiting like other Playwright assertions.

PR-#3160

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
## Issue description
The newly added geometry assertion in the Playwright spec calls `boundingBox()` on `navigation` and `firstSessionControl` immediately after switching to project-grouped view, without waiting for either locator to be visible. `boundingBox()` returns `null` for elements that are not currently visible/rendered instead of auto-waiting, so this can produce a `null` value and fail the `not.toBeNull()` assertions intermittently under CI timing.
## Issue Context
The project row's collapsible session subtree is mounted/expanded asynchronously after clicking the '按项目' radio control. The test needs stable geometry data for both the project navigation control and the first session control before asserting the indentation.
## Fix Focus Areas
- apps/desktop/e2e/sidebar-project-row.spec.ts[37-43]
Add `await expect(navigation).toBeVisible()` and `await expect(firstSessionControl).toBeVisible()` before the `Promise.all` call that reads `boundingBox()` on both locators.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context
Review mode: 🚀 Fast: This is a localized, low-risk CSS hierarchy correction with focused contract and geometry tests, avoiding security, API, state, or cross-cutting architectural changes.

Grey Divider

Tip of the day
💡 Did you know, you can route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@ARE404
ARE404force-pushed the codex/fix-sidebar-session-indentation branch from e1906de to 2478d5aCompareAugust 18, 2026 03:05
@Astro-Han

Astro-Han commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Thanks — the fix is correct and minimal: the old padding-inline-start: 0 !important at sidebar.css:237-240 genuinely deviated from the design system (Astryx's default childrenInner is already --spacing-6/24px), and the change is exactly one CSS value plus comment correction and tests. The !important necessity check is accurate (Astryx Stylex classes carry (3,1,0) specificity via :not(#\#) — Maka's (0,2,2) loses without it), and the > div > [role="group"] > div combinator chain lands only on childrenInner without hitting deeper nesting. Both the new unit test and the e2e geometry assertion run in CI (both green). Time-sort rows keeping the left edge while project-sort rows inset 24px is a stated, documented choice — fine as a product decision.

Conclusion: PASS — all P3, nothing blocking.

P3-1 — the contract test pins the mechanism, not the visual result: if someone later deletes this now-redundant override (pixel-identical), the regex match fails and assert.ok(projectChildrenRule) goes red — the test would block a zero-pixel simplification. Conversely, if Astryx changes its own nesting step in the future, the !important override keeps Maka silently stuck at 24px with all tests green, and the contract drifts from the design system. A computed-style assertion would pin the visual outcome instead.

P3-2 — the unit test's candidate-path fallback has a dead branch: in the desktop build dist/renderer/styles/sidebar.css doesn't exist (renderer goes through vite bundle), so the dist run always falls to the second candidate reading the source; the first candidate implies an intent that doesn't exist. Dropping the dead candidate (or asserting against the vite artifact explicitly) would make the intent clear.

P3-3 — the e2e threshold toBeGreaterThanOrEqual(20) at e2e/sidebar-project-row.spec.ts:44 doesn't reference --spacing-6 (24px) with a tolerance note; it only guards against "zeroed", and a spacing-5 change would pass it. A one-line comment tying it to the token would help. Also, the PR body says "Electron sidebar test" but it's a Playwright spec — minor description nit.


AI-assisted review disclosure: this review was produced with AI assistance (pi review subagent on opencode-go/deepseek-v4-flash). The subagent verified specificity math, the descendant combinator chain, and CI execution paths statically; P3 items are static observations, not observed failures. Please weigh these findings with your own judgment.

中文摘要(AI 辅助审查)

结论:PASS,全 P3。问题真实(main 上 padding-inline-start: 0 !important 偏离设计系统,Astryx 默认 childrenInner 本就是 --spacing-6/24px),改动最小(唯一行为变更是一个 CSS 值 0→var(--spacing-6),其余是注释校正与测试)。!important 必要性判断准确(Astryx Stylex 类经 :not(##)×3 特异性达 (3,1,0),Maka (0,2,2) 不加 !important 会输);子组合器链只落到 childrenInner 不误伤更深嵌套。单测 + e2e 都在 CI 真实执行且全绿。P3-1:契约测试钉的是实现机制而非视觉结果——将来删掉这条像素零变化的多余 override 会被测试拦下;反向若 Astryx 改了自己的嵌套步进,!important 会让 Maka 静默停在 24px 且测试全绿,契约与设计系统悄悄脱节(建议改用 computed-style 断言)。P3-2:单测路径回退有死分支(desktop 构建里 dist/renderer/styles/sidebar.css 不存在,dist 运行态必落第二个 candidate 读源码)。P3-3:e2e 阈值 20 是魔法数,未指向 --spacing-6(24px) 容差;PR 描述称 Electron sidebar test 实为 Playwright spec。附注:时间排序贴左缘 vs 项目排序 24px inset 是明示取舍,属产品判断。

@Astro-Han
Astro-Han merged commit 5db971a into apache:mainAug 18, 2026
12 checks passed
@ARE404
ARE404 deleted the codex/fix-sidebar-session-indentation branch August 23, 2026 08:05
Sign up for freeto 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.

2 participants

@ARE404@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

fix(desktop): indent project session rows - #3175

Merged
Astro-Han merged 2 commits into
apache:mainfrom
ARE404:codex/fix-sidebar-session-indentation
Aug 18, 2026
Merged

fix(desktop): indent project session rows#3175
Astro-Han merged 2 commits into
apache:mainfrom
ARE404:codex/fix-sidebar-session-indentation

Conversation

@ARE404

@ARE404ARE404 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Restore the standard spacing-6 (24px) SideNav nesting step for sessions rendered under a project. The CSS contract test pins the hierarchy, and the Electron test verifies the visible indentation after waiting for both measured controls to become visible.

Verification

  • npx biome check apps/desktop/src/renderer/styles/sidebar.css packages/ui/src/session-history-list.tsx apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts apps/desktop/e2e/sidebar-project-row.spec.ts — passed
  • node --test apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts — 1 passed
  • npx playwright test --config e2e/playwright.config.ts e2e/sidebar-project-row.spec.ts — 3 passed
  • Desktop build and focused compiled contract tests passed before review

Review focus

The implementation restores the existing design-system spacing token rather than introducing a new value. The geometry assertion waits for the project and first-session controls to be visible before reading boundingBox(), addressing the review reliability concern.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex audited the sidebar hierarchy, implemented and tested the CSS/E2E fix, incorporated review feedback, and drafted this description. The human contributor reviewed the work, chose to submit it, and remains responsible for its accuracy, provenance, and licensing.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@coderabbitai

coderabbitaiBot commented Aug 17, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 93f15545-8c06-4318-bc6b-27b2be7308e2

📥 Commits

Reviewing files that changed from the base of the PR and between 5d9ce0d and e1906de.

📒 Files selected for processing (4)
  • apps/desktop/e2e/sidebar-project-row.spec.ts
  • apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts
  • apps/desktop/src/renderer/styles/sidebar.css
  • packages/ui/src/session-history-list.tsx

Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.


📝 Walkthrough

What this PR solves

Project session rows in the desktop sidebar lost their normal nesting indentation because the child container applied 0 !important. This PR restores the standard spacing-6 24px indentation. It also preserves selected-state inset and right-edge alignment.

Source of truth

The PR extends the existing sidebar CSS behavior. It does not create a parallel rendering path or public API. The contract test reads sidebar.css and verifies the required hierarchy rule.

Solution scope and complexity

The change is the smallest coherent solution:

  • Remove the 0 !important indentation override.
  • Add the required --spacing-6 offset.
  • Update the related component comment.
  • Add focused CSS contract and Electron geometry coverage.

The added tests are necessary to prevent both stylesheet regressions and visible layout regressions. No added implementation complexity appears unnecessary.

Simplification opportunities

No code or test can be removed without weakening either the CSS contract or the user-visible geometry coverage. The test setup could only be simplified if another existing test already provided equivalent coverage for both the rule and rendered spacing.

Risks and validation

The change affects desktop sidebar layout. Session rows now appear 24px farther from the project row. Incorrect CSS specificity could still prevent the intended rule from applying.

The PR adds:

  • A CSS contract test for the explicit 24px hierarchy rule.
  • An Electron test that waits for sidebar geometry and verifies at least 20px of horizontal separation.
  • Existing validation reported by the PR: desktop builds, unit tests, Playwright tests, Biome checks, and layout detection.

The final status of required checks is unverified from the available evidence.

Review-relevant risks

The current diff has a user-visible desktop layout effect. Material changes in user-visible behavior require independent human review under repository policy.

No public contract, security, licensing, release, or governance effect was identified in the current diff.

The person performing the merge must review the final diff. A maintainer makes the final determination.

Walkthrough

Project child session rows now render with a 24px nesting offset. Contract and end-to-end tests verify the stylesheet rule and horizontal separation.

Changes

Sidebar hierarchy

Layer / File(s)Summary
Project session indentation and validation
apps/desktop/src/renderer/styles/sidebar.css, packages/ui/src/session-history-list.tsx, apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts, apps/desktop/e2e/sidebar-project-row.spec.ts
Project child sessions use var(--spacing-6) inline-start padding. The related comment describes the nesting. Contract tests verify the CSS rule, and end-to-end tests verify visible horizontal separation.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk:⚪ Minimal · up to e1906

This PR restores project/session indentation and adds targeted regression coverage; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers:astro-han, jackwener

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Ai Use Disclosure⚠️ WarningThe PR description selects neither AI-use declaration, and both introduced commits have no Generated-by trailer.Select exactly one declaration and, if applicable, name the tool and scope. Follow CONTRIBUTING.md “Human ownership and AI attribution”; retain required trailers through squash or amend.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.
Description check✅ PassedThe description includes the required summary, verification, AI use, and checklist sections with specific implementation details and test results.
Title check✅ PassedThe title clearly and concisely describes the main change: restoring indentation for project session rows.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@ARE404
ARE404 marked this pull request as ready for review August 17, 2026 16:11
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix desktop sidebar: indent project session rows (restore spacing-6)

🐞 Bug fix🧪 Tests🕐 20-40 Minutes

Grey Divider

AI Description

• Restore Astryx SideNav child indentation (spacing-6 / 24px) for sessions under projects.
• Add a CSS contract test to pin the project→session hierarchy rule.
• Strengthen the Electron sidebar Playwright test with a geometry-based indentation assertion.
Diagram

graph TD
A["ProjectNavRow (ui)"] --> B["Electron renderer"] --> C["sidebar.css rule"]
D["CSS contract test"] --> C
E["Playwright sidebar e2e"] --> B
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Move indentation control into the component (StyleX)
  • ➕ Eliminates reliance on unlayered CSS overrides + !important
  • ➕ Keeps hierarchy styling co-located with the rendering logic
  • ➖ May require StyleX layering/ordering changes that are broader than this fix
  • ➖ Harder to validate across build outputs if CSS extraction differs by environment
2. Rely on Playwright geometry test only (drop CSS contract test)
  • ➕ Less brittle than regex-matching a CSS selector/body
  • ➕ Ensures the user-visible outcome is correct
  • ➖ Slower and potentially flakier than a pure unit/contract test
  • ➖ Harder to pinpoint regressions (CSS rule vs layout/DOM change)

Recommendation: Current approach (restore the design-system spacing token in sidebar.css, plus a fast CSS contract test and a user-visible geometry assertion) is the best tradeoff: it fixes the regression with minimal surface area, locks in the intended hierarchy at two levels (source CSS and rendered layout), and avoids introducing new spacing values.

Files changed (4) +44 / -8

Bug fix (1) +8 / -7
sidebar.cssRestore spacing-6 indentation for project child session container+8/-7

Restore spacing-6 indentation for project child session container

• Updates the .maka-project-row child-group rule to apply padding-inline-start: var(--spacing-6) !important instead of zeroing indentation. Rewrites the comment to reflect that project grouping is a true parent-child hierarchy and why the value is pinned.

apps/desktop/src/renderer/styles/sidebar.css

Tests (2) +35 / -0
sidebar-project-row.spec.tsAdd geometry assertion for project→session indentation+8/-0

Add geometry assertion for project→session indentation

• Captures bounding boxes for the project navigation control and the first session control, asserting the session is horizontally offset. This makes the indentation regression detectable in the Electron E2E suite rather than relying only on focus/order assertions.

apps/desktop/e2e/sidebar-project-row.spec.ts

session-project-hierarchy-contract.test.tsAdd contract test pinning sidebar CSS hierarchy rule+27/-0

Add contract test pinning sidebar CSS hierarchy rule

• Introduces a Node test that locates sidebar.css across source/dist paths and asserts the presence of a specific project-children selector. Pins the required padding-inline-start to var(--spacing-6) with !important as a visual contract.

apps/desktop/src/main/tests/session-project-hierarchy-contract.test.ts

Documentation (1) +1 / -1
session-history-list.tsxUpdate ProjectNavRow comment to match indentation behavior+1/-1

Update ProjectNavRow comment to match indentation behavior

• Adjusts the inline comment above the nested session rendering to reflect that sidebar.css preserves a standard SideNav nesting step for the project hierarchy.

packages/ui/src/session-history-list.tsx

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1)📘 Rule violations (0)📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Geometry assertion races visibility 🐞 Bug☼ Reliability
Description
The new test samples navigation.boundingBox() and firstSessionControl.boundingBox() immediately
after switching to project mode, without waiting for either element to be visible first.
boundingBox() does not auto-wait for visibility like assertion matchers do, so it can return
null while the sidebar is still re-rendering, making the new non-null assertions intermittently
fail in CI.
Code

apps/desktop/e2e/sidebar-project-row.spec.ts[R37-43]

+ const [projectNavigationBox, firstSessionBox] = await Promise.all([+ navigation.boundingBox(),+ firstSessionControl.boundingBox(),+ ]);+ expect(projectNavigationBox).not.toBeNull();+ expect(firstSessionBox).not.toBeNull();+ expect(firstSessionBox!.x - projectNavigationBox!.x).toBeGreaterThanOrEqual(20);
Relevance

●●● Strong

Matching accepted precedent explicitly requires visibility waits before boundingBox after UI
transitions.

PR-#3160

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The project-mode radio is clicked at line 20 and the DOM updates asynchronously
(aria-expanded/aria-controls wiring, session subtree mount). The new code at lines 37-43 immediately
calls boundingBox() on both locators and asserts non-null without first waiting for visibility
(e.g., via toBeVisible()), which is a documented gap since boundingBox() returns null for elements
not currently rendered/visible instead of waiting like other Playwright assertions.

PR-#3160

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
## Issue description
The newly added geometry assertion in the Playwright spec calls `boundingBox()` on `navigation` and `firstSessionControl` immediately after switching to project-grouped view, without waiting for either locator to be visible. `boundingBox()` returns `null` for elements that are not currently visible/rendered instead of auto-waiting, so this can produce a `null` value and fail the `not.toBeNull()` assertions intermittently under CI timing.
## Issue Context
The project row's collapsible session subtree is mounted/expanded asynchronously after clicking the '按项目' radio control. The test needs stable geometry data for both the project navigation control and the first session control before asserting the indentation.
## Fix Focus Areas
- apps/desktop/e2e/sidebar-project-row.spec.ts[37-43]
Add `await expect(navigation).toBeVisible()` and `await expect(firstSessionControl).toBeVisible()` before the `Promise.all` call that reads `boundingBox()` on both locators.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context
Review mode: 🚀 Fast: This is a localized, low-risk CSS hierarchy correction with focused contract and geometry tests, avoiding security, API, state, or cross-cutting architectural changes.

Grey Divider

Tip of the day
💡 Did you know, you can route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@ARE404
ARE404force-pushed the codex/fix-sidebar-session-indentation branch from e1906de to 2478d5aCompareAugust 18, 2026 03:05
@Astro-Han

Astro-Han commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Thanks — the fix is correct and minimal: the old padding-inline-start: 0 !important at sidebar.css:237-240 genuinely deviated from the design system (Astryx's default childrenInner is already --spacing-6/24px), and the change is exactly one CSS value plus comment correction and tests. The !important necessity check is accurate (Astryx Stylex classes carry (3,1,0) specificity via :not(#\#) — Maka's (0,2,2) loses without it), and the > div > [role="group"] > div combinator chain lands only on childrenInner without hitting deeper nesting. Both the new unit test and the e2e geometry assertion run in CI (both green). Time-sort rows keeping the left edge while project-sort rows inset 24px is a stated, documented choice — fine as a product decision.

Conclusion: PASS — all P3, nothing blocking.

P3-1 — the contract test pins the mechanism, not the visual result: if someone later deletes this now-redundant override (pixel-identical), the regex match fails and assert.ok(projectChildrenRule) goes red — the test would block a zero-pixel simplification. Conversely, if Astryx changes its own nesting step in the future, the !important override keeps Maka silently stuck at 24px with all tests green, and the contract drifts from the design system. A computed-style assertion would pin the visual outcome instead.

P3-2 — the unit test's candidate-path fallback has a dead branch: in the desktop build dist/renderer/styles/sidebar.css doesn't exist (renderer goes through vite bundle), so the dist run always falls to the second candidate reading the source; the first candidate implies an intent that doesn't exist. Dropping the dead candidate (or asserting against the vite artifact explicitly) would make the intent clear.

P3-3 — the e2e threshold toBeGreaterThanOrEqual(20) at e2e/sidebar-project-row.spec.ts:44 doesn't reference --spacing-6 (24px) with a tolerance note; it only guards against "zeroed", and a spacing-5 change would pass it. A one-line comment tying it to the token would help. Also, the PR body says "Electron sidebar test" but it's a Playwright spec — minor description nit.


AI-assisted review disclosure: this review was produced with AI assistance (pi review subagent on opencode-go/deepseek-v4-flash). The subagent verified specificity math, the descendant combinator chain, and CI execution paths statically; P3 items are static observations, not observed failures. Please weigh these findings with your own judgment.

中文摘要(AI 辅助审查)

结论:PASS,全 P3。问题真实(main 上 padding-inline-start: 0 !important 偏离设计系统,Astryx 默认 childrenInner 本就是 --spacing-6/24px),改动最小(唯一行为变更是一个 CSS 值 0→var(--spacing-6),其余是注释校正与测试)。!important 必要性判断准确(Astryx Stylex 类经 :not(##)×3 特异性达 (3,1,0),Maka (0,2,2) 不加 !important 会输);子组合器链只落到 childrenInner 不误伤更深嵌套。单测 + e2e 都在 CI 真实执行且全绿。P3-1:契约测试钉的是实现机制而非视觉结果——将来删掉这条像素零变化的多余 override 会被测试拦下;反向若 Astryx 改了自己的嵌套步进,!important 会让 Maka 静默停在 24px 且测试全绿,契约与设计系统悄悄脱节(建议改用 computed-style 断言)。P3-2:单测路径回退有死分支(desktop 构建里 dist/renderer/styles/sidebar.css 不存在,dist 运行态必落第二个 candidate 读源码)。P3-3:e2e 阈值 20 是魔法数,未指向 --spacing-6(24px) 容差;PR 描述称 Electron sidebar test 实为 Playwright spec。附注:时间排序贴左缘 vs 项目排序 24px inset 是明示取舍,属产品判断。

@Astro-Han
Astro-Han merged commit 5db971a into apache:mainAug 18, 2026
12 checks passed
@ARE404
ARE404 deleted the codex/fix-sidebar-session-indentation branch August 23, 2026 08:05
Sign up for freeto 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.

2 participants

@ARE404@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(desktop): indent project session rows - #3175

Merged
Astro-Han merged 2 commits into
apache:mainfrom
ARE404:codex/fix-sidebar-session-indentation
Aug 18, 2026
Merged

fix(desktop): indent project session rows#3175
Astro-Han merged 2 commits into
apache:mainfrom
ARE404:codex/fix-sidebar-session-indentation

Conversation

@ARE404

@ARE404ARE404 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Restore the standard spacing-6 (24px) SideNav nesting step for sessions rendered under a project. The CSS contract test pins the hierarchy, and the Electron test verifies the visible indentation after waiting for both measured controls to become visible.

Verification

  • npx biome check apps/desktop/src/renderer/styles/sidebar.css packages/ui/src/session-history-list.tsx apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts apps/desktop/e2e/sidebar-project-row.spec.ts — passed
  • node --test apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts — 1 passed
  • npx playwright test --config e2e/playwright.config.ts e2e/sidebar-project-row.spec.ts — 3 passed
  • Desktop build and focused compiled contract tests passed before review

Review focus

The implementation restores the existing design-system spacing token rather than introducing a new value. The geometry assertion waits for the project and first-session controls to be visible before reading boundingBox(), addressing the review reliability concern.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex audited the sidebar hierarchy, implemented and tested the CSS/E2E fix, incorporated review feedback, and drafted this description. The human contributor reviewed the work, chose to submit it, and remains responsible for its accuracy, provenance, and licensing.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@coderabbitai

coderabbitaiBot commented Aug 17, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 93f15545-8c06-4318-bc6b-27b2be7308e2

📥 Commits

Reviewing files that changed from the base of the PR and between 5d9ce0d and e1906de.

📒 Files selected for processing (4)
  • apps/desktop/e2e/sidebar-project-row.spec.ts
  • apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts
  • apps/desktop/src/renderer/styles/sidebar.css
  • packages/ui/src/session-history-list.tsx

Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.


📝 Walkthrough

What this PR solves

Project session rows in the desktop sidebar lost their normal nesting indentation because the child container applied 0 !important. This PR restores the standard spacing-6 24px indentation. It also preserves selected-state inset and right-edge alignment.

Source of truth

The PR extends the existing sidebar CSS behavior. It does not create a parallel rendering path or public API. The contract test reads sidebar.css and verifies the required hierarchy rule.

Solution scope and complexity

The change is the smallest coherent solution:

  • Remove the 0 !important indentation override.
  • Add the required --spacing-6 offset.
  • Update the related component comment.
  • Add focused CSS contract and Electron geometry coverage.

The added tests are necessary to prevent both stylesheet regressions and visible layout regressions. No added implementation complexity appears unnecessary.

Simplification opportunities

No code or test can be removed without weakening either the CSS contract or the user-visible geometry coverage. The test setup could only be simplified if another existing test already provided equivalent coverage for both the rule and rendered spacing.

Risks and validation

The change affects desktop sidebar layout. Session rows now appear 24px farther from the project row. Incorrect CSS specificity could still prevent the intended rule from applying.

The PR adds:

  • A CSS contract test for the explicit 24px hierarchy rule.
  • An Electron test that waits for sidebar geometry and verifies at least 20px of horizontal separation.
  • Existing validation reported by the PR: desktop builds, unit tests, Playwright tests, Biome checks, and layout detection.

The final status of required checks is unverified from the available evidence.

Review-relevant risks

The current diff has a user-visible desktop layout effect. Material changes in user-visible behavior require independent human review under repository policy.

No public contract, security, licensing, release, or governance effect was identified in the current diff.

The person performing the merge must review the final diff. A maintainer makes the final determination.

Walkthrough

Project child session rows now render with a 24px nesting offset. Contract and end-to-end tests verify the stylesheet rule and horizontal separation.

Changes

Sidebar hierarchy

Layer / File(s)Summary
Project session indentation and validation
apps/desktop/src/renderer/styles/sidebar.css, packages/ui/src/session-history-list.tsx, apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts, apps/desktop/e2e/sidebar-project-row.spec.ts
Project child sessions use var(--spacing-6) inline-start padding. The related comment describes the nesting. Contract tests verify the CSS rule, and end-to-end tests verify visible horizontal separation.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk:⚪ Minimal · up to e1906

This PR restores project/session indentation and adds targeted regression coverage; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers:astro-han, jackwener

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Ai Use Disclosure⚠️ WarningThe PR description selects neither AI-use declaration, and both introduced commits have no Generated-by trailer.Select exactly one declaration and, if applicable, name the tool and scope. Follow CONTRIBUTING.md “Human ownership and AI attribution”; retain required trailers through squash or amend.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.
Description check✅ PassedThe description includes the required summary, verification, AI use, and checklist sections with specific implementation details and test results.
Title check✅ PassedThe title clearly and concisely describes the main change: restoring indentation for project session rows.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@ARE404
ARE404 marked this pull request as ready for review August 17, 2026 16:11
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix desktop sidebar: indent project session rows (restore spacing-6)

🐞 Bug fix🧪 Tests🕐 20-40 Minutes

Grey Divider

AI Description

• Restore Astryx SideNav child indentation (spacing-6 / 24px) for sessions under projects.
• Add a CSS contract test to pin the project→session hierarchy rule.
• Strengthen the Electron sidebar Playwright test with a geometry-based indentation assertion.
Diagram

graph TD
A["ProjectNavRow (ui)"] --> B["Electron renderer"] --> C["sidebar.css rule"]
D["CSS contract test"] --> C
E["Playwright sidebar e2e"] --> B
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Move indentation control into the component (StyleX)
  • ➕ Eliminates reliance on unlayered CSS overrides + !important
  • ➕ Keeps hierarchy styling co-located with the rendering logic
  • ➖ May require StyleX layering/ordering changes that are broader than this fix
  • ➖ Harder to validate across build outputs if CSS extraction differs by environment
2. Rely on Playwright geometry test only (drop CSS contract test)
  • ➕ Less brittle than regex-matching a CSS selector/body
  • ➕ Ensures the user-visible outcome is correct
  • ➖ Slower and potentially flakier than a pure unit/contract test
  • ➖ Harder to pinpoint regressions (CSS rule vs layout/DOM change)

Recommendation: Current approach (restore the design-system spacing token in sidebar.css, plus a fast CSS contract test and a user-visible geometry assertion) is the best tradeoff: it fixes the regression with minimal surface area, locks in the intended hierarchy at two levels (source CSS and rendered layout), and avoids introducing new spacing values.

Files changed (4) +44 / -8

Bug fix (1) +8 / -7
sidebar.cssRestore spacing-6 indentation for project child session container+8/-7

Restore spacing-6 indentation for project child session container

• Updates the .maka-project-row child-group rule to apply padding-inline-start: var(--spacing-6) !important instead of zeroing indentation. Rewrites the comment to reflect that project grouping is a true parent-child hierarchy and why the value is pinned.

apps/desktop/src/renderer/styles/sidebar.css

Tests (2) +35 / -0
sidebar-project-row.spec.tsAdd geometry assertion for project→session indentation+8/-0

Add geometry assertion for project→session indentation

• Captures bounding boxes for the project navigation control and the first session control, asserting the session is horizontally offset. This makes the indentation regression detectable in the Electron E2E suite rather than relying only on focus/order assertions.

apps/desktop/e2e/sidebar-project-row.spec.ts

session-project-hierarchy-contract.test.tsAdd contract test pinning sidebar CSS hierarchy rule+27/-0

Add contract test pinning sidebar CSS hierarchy rule

• Introduces a Node test that locates sidebar.css across source/dist paths and asserts the presence of a specific project-children selector. Pins the required padding-inline-start to var(--spacing-6) with !important as a visual contract.

apps/desktop/src/main/tests/session-project-hierarchy-contract.test.ts

Documentation (1) +1 / -1
session-history-list.tsxUpdate ProjectNavRow comment to match indentation behavior+1/-1

Update ProjectNavRow comment to match indentation behavior

• Adjusts the inline comment above the nested session rendering to reflect that sidebar.css preserves a standard SideNav nesting step for the project hierarchy.

packages/ui/src/session-history-list.tsx

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1)📘 Rule violations (0)📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Geometry assertion races visibility 🐞 Bug☼ Reliability
Description
The new test samples navigation.boundingBox() and firstSessionControl.boundingBox() immediately
after switching to project mode, without waiting for either element to be visible first.
boundingBox() does not auto-wait for visibility like assertion matchers do, so it can return
null while the sidebar is still re-rendering, making the new non-null assertions intermittently
fail in CI.
Code

apps/desktop/e2e/sidebar-project-row.spec.ts[R37-43]

+ const [projectNavigationBox, firstSessionBox] = await Promise.all([+ navigation.boundingBox(),+ firstSessionControl.boundingBox(),+ ]);+ expect(projectNavigationBox).not.toBeNull();+ expect(firstSessionBox).not.toBeNull();+ expect(firstSessionBox!.x - projectNavigationBox!.x).toBeGreaterThanOrEqual(20);
Relevance

●●● Strong

Matching accepted precedent explicitly requires visibility waits before boundingBox after UI
transitions.

PR-#3160

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The project-mode radio is clicked at line 20 and the DOM updates asynchronously
(aria-expanded/aria-controls wiring, session subtree mount). The new code at lines 37-43 immediately
calls boundingBox() on both locators and asserts non-null without first waiting for visibility
(e.g., via toBeVisible()), which is a documented gap since boundingBox() returns null for elements
not currently rendered/visible instead of waiting like other Playwright assertions.

PR-#3160

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
## Issue description
The newly added geometry assertion in the Playwright spec calls `boundingBox()` on `navigation` and `firstSessionControl` immediately after switching to project-grouped view, without waiting for either locator to be visible. `boundingBox()` returns `null` for elements that are not currently visible/rendered instead of auto-waiting, so this can produce a `null` value and fail the `not.toBeNull()` assertions intermittently under CI timing.
## Issue Context
The project row's collapsible session subtree is mounted/expanded asynchronously after clicking the '按项目' radio control. The test needs stable geometry data for both the project navigation control and the first session control before asserting the indentation.
## Fix Focus Areas
- apps/desktop/e2e/sidebar-project-row.spec.ts[37-43]
Add `await expect(navigation).toBeVisible()` and `await expect(firstSessionControl).toBeVisible()` before the `Promise.all` call that reads `boundingBox()` on both locators.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context
Review mode: 🚀 Fast: This is a localized, low-risk CSS hierarchy correction with focused contract and geometry tests, avoiding security, API, state, or cross-cutting architectural changes.

Grey Divider

Tip of the day
💡 Did you know, you can route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@ARE404
ARE404force-pushed the codex/fix-sidebar-session-indentation branch from e1906de to 2478d5aCompareAugust 18, 2026 03:05
@Astro-Han

Astro-Han commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Thanks — the fix is correct and minimal: the old padding-inline-start: 0 !important at sidebar.css:237-240 genuinely deviated from the design system (Astryx's default childrenInner is already --spacing-6/24px), and the change is exactly one CSS value plus comment correction and tests. The !important necessity check is accurate (Astryx Stylex classes carry (3,1,0) specificity via :not(#\#) — Maka's (0,2,2) loses without it), and the > div > [role="group"] > div combinator chain lands only on childrenInner without hitting deeper nesting. Both the new unit test and the e2e geometry assertion run in CI (both green). Time-sort rows keeping the left edge while project-sort rows inset 24px is a stated, documented choice — fine as a product decision.

Conclusion: PASS — all P3, nothing blocking.

P3-1 — the contract test pins the mechanism, not the visual result: if someone later deletes this now-redundant override (pixel-identical), the regex match fails and assert.ok(projectChildrenRule) goes red — the test would block a zero-pixel simplification. Conversely, if Astryx changes its own nesting step in the future, the !important override keeps Maka silently stuck at 24px with all tests green, and the contract drifts from the design system. A computed-style assertion would pin the visual outcome instead.

P3-2 — the unit test's candidate-path fallback has a dead branch: in the desktop build dist/renderer/styles/sidebar.css doesn't exist (renderer goes through vite bundle), so the dist run always falls to the second candidate reading the source; the first candidate implies an intent that doesn't exist. Dropping the dead candidate (or asserting against the vite artifact explicitly) would make the intent clear.

P3-3 — the e2e threshold toBeGreaterThanOrEqual(20) at e2e/sidebar-project-row.spec.ts:44 doesn't reference --spacing-6 (24px) with a tolerance note; it only guards against "zeroed", and a spacing-5 change would pass it. A one-line comment tying it to the token would help. Also, the PR body says "Electron sidebar test" but it's a Playwright spec — minor description nit.


AI-assisted review disclosure: this review was produced with AI assistance (pi review subagent on opencode-go/deepseek-v4-flash). The subagent verified specificity math, the descendant combinator chain, and CI execution paths statically; P3 items are static observations, not observed failures. Please weigh these findings with your own judgment.

中文摘要(AI 辅助审查)

结论:PASS,全 P3。问题真实(main 上 padding-inline-start: 0 !important 偏离设计系统,Astryx 默认 childrenInner 本就是 --spacing-6/24px),改动最小(唯一行为变更是一个 CSS 值 0→var(--spacing-6),其余是注释校正与测试)。!important 必要性判断准确(Astryx Stylex 类经 :not(##)×3 特异性达 (3,1,0),Maka (0,2,2) 不加 !important 会输);子组合器链只落到 childrenInner 不误伤更深嵌套。单测 + e2e 都在 CI 真实执行且全绿。P3-1:契约测试钉的是实现机制而非视觉结果——将来删掉这条像素零变化的多余 override 会被测试拦下;反向若 Astryx 改了自己的嵌套步进,!important 会让 Maka 静默停在 24px 且测试全绿,契约与设计系统悄悄脱节(建议改用 computed-style 断言)。P3-2:单测路径回退有死分支(desktop 构建里 dist/renderer/styles/sidebar.css 不存在,dist 运行态必落第二个 candidate 读源码)。P3-3:e2e 阈值 20 是魔法数,未指向 --spacing-6(24px) 容差;PR 描述称 Electron sidebar test 实为 Playwright spec。附注:时间排序贴左缘 vs 项目排序 24px inset 是明示取舍,属产品判断。

@Astro-Han
Astro-Han merged commit 5db971a into apache:mainAug 18, 2026
12 checks passed
@ARE404
ARE404 deleted the codex/fix-sidebar-session-indentation branch August 23, 2026 08:05
Sign up for freeto 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.

2 participants

@ARE404@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(desktop): indent project session rows - #3175

Merged
Astro-Han merged 2 commits into
apache:mainfrom
ARE404:codex/fix-sidebar-session-indentation
Aug 18, 2026
Merged

fix(desktop): indent project session rows#3175
Astro-Han merged 2 commits into
apache:mainfrom
ARE404:codex/fix-sidebar-session-indentation

Conversation

@ARE404

@ARE404ARE404 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Restore the standard spacing-6 (24px) SideNav nesting step for sessions rendered under a project. The CSS contract test pins the hierarchy, and the Electron test verifies the visible indentation after waiting for both measured controls to become visible.

Verification

  • npx biome check apps/desktop/src/renderer/styles/sidebar.css packages/ui/src/session-history-list.tsx apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts apps/desktop/e2e/sidebar-project-row.spec.ts — passed
  • node --test apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts — 1 passed
  • npx playwright test --config e2e/playwright.config.ts e2e/sidebar-project-row.spec.ts — 3 passed
  • Desktop build and focused compiled contract tests passed before review

Review focus

The implementation restores the existing design-system spacing token rather than introducing a new value. The geometry assertion waits for the project and first-session controls to be visible before reading boundingBox(), addressing the review reliability concern.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex audited the sidebar hierarchy, implemented and tested the CSS/E2E fix, incorporated review feedback, and drafted this description. The human contributor reviewed the work, chose to submit it, and remains responsible for its accuracy, provenance, and licensing.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@coderabbitai

coderabbitaiBot commented Aug 17, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 93f15545-8c06-4318-bc6b-27b2be7308e2

📥 Commits

Reviewing files that changed from the base of the PR and between 5d9ce0d and e1906de.

📒 Files selected for processing (4)
  • apps/desktop/e2e/sidebar-project-row.spec.ts
  • apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts
  • apps/desktop/src/renderer/styles/sidebar.css
  • packages/ui/src/session-history-list.tsx

Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.


📝 Walkthrough

What this PR solves

Project session rows in the desktop sidebar lost their normal nesting indentation because the child container applied 0 !important. This PR restores the standard spacing-6 24px indentation. It also preserves selected-state inset and right-edge alignment.

Source of truth

The PR extends the existing sidebar CSS behavior. It does not create a parallel rendering path or public API. The contract test reads sidebar.css and verifies the required hierarchy rule.

Solution scope and complexity

The change is the smallest coherent solution:

  • Remove the 0 !important indentation override.
  • Add the required --spacing-6 offset.
  • Update the related component comment.
  • Add focused CSS contract and Electron geometry coverage.

The added tests are necessary to prevent both stylesheet regressions and visible layout regressions. No added implementation complexity appears unnecessary.

Simplification opportunities

No code or test can be removed without weakening either the CSS contract or the user-visible geometry coverage. The test setup could only be simplified if another existing test already provided equivalent coverage for both the rule and rendered spacing.

Risks and validation

The change affects desktop sidebar layout. Session rows now appear 24px farther from the project row. Incorrect CSS specificity could still prevent the intended rule from applying.

The PR adds:

  • A CSS contract test for the explicit 24px hierarchy rule.
  • An Electron test that waits for sidebar geometry and verifies at least 20px of horizontal separation.
  • Existing validation reported by the PR: desktop builds, unit tests, Playwright tests, Biome checks, and layout detection.

The final status of required checks is unverified from the available evidence.

Review-relevant risks

The current diff has a user-visible desktop layout effect. Material changes in user-visible behavior require independent human review under repository policy.

No public contract, security, licensing, release, or governance effect was identified in the current diff.

The person performing the merge must review the final diff. A maintainer makes the final determination.

Walkthrough

Project child session rows now render with a 24px nesting offset. Contract and end-to-end tests verify the stylesheet rule and horizontal separation.

Changes

Sidebar hierarchy

Layer / File(s)Summary
Project session indentation and validation
apps/desktop/src/renderer/styles/sidebar.css, packages/ui/src/session-history-list.tsx, apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts, apps/desktop/e2e/sidebar-project-row.spec.ts
Project child sessions use var(--spacing-6) inline-start padding. The related comment describes the nesting. Contract tests verify the CSS rule, and end-to-end tests verify visible horizontal separation.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk:⚪ Minimal · up to e1906

This PR restores project/session indentation and adds targeted regression coverage; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers:astro-han, jackwener

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Ai Use Disclosure⚠️ WarningThe PR description selects neither AI-use declaration, and both introduced commits have no Generated-by trailer.Select exactly one declaration and, if applicable, name the tool and scope. Follow CONTRIBUTING.md “Human ownership and AI attribution”; retain required trailers through squash or amend.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.
Description check✅ PassedThe description includes the required summary, verification, AI use, and checklist sections with specific implementation details and test results.
Title check✅ PassedThe title clearly and concisely describes the main change: restoring indentation for project session rows.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@ARE404
ARE404 marked this pull request as ready for review August 17, 2026 16:11
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix desktop sidebar: indent project session rows (restore spacing-6)

🐞 Bug fix🧪 Tests🕐 20-40 Minutes

Grey Divider

AI Description

• Restore Astryx SideNav child indentation (spacing-6 / 24px) for sessions under projects.
• Add a CSS contract test to pin the project→session hierarchy rule.
• Strengthen the Electron sidebar Playwright test with a geometry-based indentation assertion.
Diagram

graph TD
A["ProjectNavRow (ui)"] --> B["Electron renderer"] --> C["sidebar.css rule"]
D["CSS contract test"] --> C
E["Playwright sidebar e2e"] --> B
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Move indentation control into the component (StyleX)
  • ➕ Eliminates reliance on unlayered CSS overrides + !important
  • ➕ Keeps hierarchy styling co-located with the rendering logic
  • ➖ May require StyleX layering/ordering changes that are broader than this fix
  • ➖ Harder to validate across build outputs if CSS extraction differs by environment
2. Rely on Playwright geometry test only (drop CSS contract test)
  • ➕ Less brittle than regex-matching a CSS selector/body
  • ➕ Ensures the user-visible outcome is correct
  • ➖ Slower and potentially flakier than a pure unit/contract test
  • ➖ Harder to pinpoint regressions (CSS rule vs layout/DOM change)

Recommendation: Current approach (restore the design-system spacing token in sidebar.css, plus a fast CSS contract test and a user-visible geometry assertion) is the best tradeoff: it fixes the regression with minimal surface area, locks in the intended hierarchy at two levels (source CSS and rendered layout), and avoids introducing new spacing values.

Files changed (4) +44 / -8

Bug fix (1) +8 / -7
sidebar.cssRestore spacing-6 indentation for project child session container+8/-7

Restore spacing-6 indentation for project child session container

• Updates the .maka-project-row child-group rule to apply padding-inline-start: var(--spacing-6) !important instead of zeroing indentation. Rewrites the comment to reflect that project grouping is a true parent-child hierarchy and why the value is pinned.

apps/desktop/src/renderer/styles/sidebar.css

Tests (2) +35 / -0
sidebar-project-row.spec.tsAdd geometry assertion for project→session indentation+8/-0

Add geometry assertion for project→session indentation

• Captures bounding boxes for the project navigation control and the first session control, asserting the session is horizontally offset. This makes the indentation regression detectable in the Electron E2E suite rather than relying only on focus/order assertions.

apps/desktop/e2e/sidebar-project-row.spec.ts

session-project-hierarchy-contract.test.tsAdd contract test pinning sidebar CSS hierarchy rule+27/-0

Add contract test pinning sidebar CSS hierarchy rule

• Introduces a Node test that locates sidebar.css across source/dist paths and asserts the presence of a specific project-children selector. Pins the required padding-inline-start to var(--spacing-6) with !important as a visual contract.

apps/desktop/src/main/tests/session-project-hierarchy-contract.test.ts

Documentation (1) +1 / -1
session-history-list.tsxUpdate ProjectNavRow comment to match indentation behavior+1/-1

Update ProjectNavRow comment to match indentation behavior

• Adjusts the inline comment above the nested session rendering to reflect that sidebar.css preserves a standard SideNav nesting step for the project hierarchy.

packages/ui/src/session-history-list.tsx

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1)📘 Rule violations (0)📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Geometry assertion races visibility 🐞 Bug☼ Reliability
Description
The new test samples navigation.boundingBox() and firstSessionControl.boundingBox() immediately
after switching to project mode, without waiting for either element to be visible first.
boundingBox() does not auto-wait for visibility like assertion matchers do, so it can return
null while the sidebar is still re-rendering, making the new non-null assertions intermittently
fail in CI.
Code

apps/desktop/e2e/sidebar-project-row.spec.ts[R37-43]

+ const [projectNavigationBox, firstSessionBox] = await Promise.all([+ navigation.boundingBox(),+ firstSessionControl.boundingBox(),+ ]);+ expect(projectNavigationBox).not.toBeNull();+ expect(firstSessionBox).not.toBeNull();+ expect(firstSessionBox!.x - projectNavigationBox!.x).toBeGreaterThanOrEqual(20);
Relevance

●●● Strong

Matching accepted precedent explicitly requires visibility waits before boundingBox after UI
transitions.

PR-#3160

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The project-mode radio is clicked at line 20 and the DOM updates asynchronously
(aria-expanded/aria-controls wiring, session subtree mount). The new code at lines 37-43 immediately
calls boundingBox() on both locators and asserts non-null without first waiting for visibility
(e.g., via toBeVisible()), which is a documented gap since boundingBox() returns null for elements
not currently rendered/visible instead of waiting like other Playwright assertions.

PR-#3160

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
## Issue description
The newly added geometry assertion in the Playwright spec calls `boundingBox()` on `navigation` and `firstSessionControl` immediately after switching to project-grouped view, without waiting for either locator to be visible. `boundingBox()` returns `null` for elements that are not currently visible/rendered instead of auto-waiting, so this can produce a `null` value and fail the `not.toBeNull()` assertions intermittently under CI timing.
## Issue Context
The project row's collapsible session subtree is mounted/expanded asynchronously after clicking the '按项目' radio control. The test needs stable geometry data for both the project navigation control and the first session control before asserting the indentation.
## Fix Focus Areas
- apps/desktop/e2e/sidebar-project-row.spec.ts[37-43]
Add `await expect(navigation).toBeVisible()` and `await expect(firstSessionControl).toBeVisible()` before the `Promise.all` call that reads `boundingBox()` on both locators.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context
Review mode: 🚀 Fast: This is a localized, low-risk CSS hierarchy correction with focused contract and geometry tests, avoiding security, API, state, or cross-cutting architectural changes.

Grey Divider

Tip of the day
💡 Did you know, you can route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@ARE404
ARE404force-pushed the codex/fix-sidebar-session-indentation branch from e1906de to 2478d5aCompareAugust 18, 2026 03:05
@Astro-Han

Astro-Han commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Thanks — the fix is correct and minimal: the old padding-inline-start: 0 !important at sidebar.css:237-240 genuinely deviated from the design system (Astryx's default childrenInner is already --spacing-6/24px), and the change is exactly one CSS value plus comment correction and tests. The !important necessity check is accurate (Astryx Stylex classes carry (3,1,0) specificity via :not(#\#) — Maka's (0,2,2) loses without it), and the > div > [role="group"] > div combinator chain lands only on childrenInner without hitting deeper nesting. Both the new unit test and the e2e geometry assertion run in CI (both green). Time-sort rows keeping the left edge while project-sort rows inset 24px is a stated, documented choice — fine as a product decision.

Conclusion: PASS — all P3, nothing blocking.

P3-1 — the contract test pins the mechanism, not the visual result: if someone later deletes this now-redundant override (pixel-identical), the regex match fails and assert.ok(projectChildrenRule) goes red — the test would block a zero-pixel simplification. Conversely, if Astryx changes its own nesting step in the future, the !important override keeps Maka silently stuck at 24px with all tests green, and the contract drifts from the design system. A computed-style assertion would pin the visual outcome instead.

P3-2 — the unit test's candidate-path fallback has a dead branch: in the desktop build dist/renderer/styles/sidebar.css doesn't exist (renderer goes through vite bundle), so the dist run always falls to the second candidate reading the source; the first candidate implies an intent that doesn't exist. Dropping the dead candidate (or asserting against the vite artifact explicitly) would make the intent clear.

P3-3 — the e2e threshold toBeGreaterThanOrEqual(20) at e2e/sidebar-project-row.spec.ts:44 doesn't reference --spacing-6 (24px) with a tolerance note; it only guards against "zeroed", and a spacing-5 change would pass it. A one-line comment tying it to the token would help. Also, the PR body says "Electron sidebar test" but it's a Playwright spec — minor description nit.


AI-assisted review disclosure: this review was produced with AI assistance (pi review subagent on opencode-go/deepseek-v4-flash). The subagent verified specificity math, the descendant combinator chain, and CI execution paths statically; P3 items are static observations, not observed failures. Please weigh these findings with your own judgment.

中文摘要(AI 辅助审查)

结论:PASS,全 P3。问题真实(main 上 padding-inline-start: 0 !important 偏离设计系统,Astryx 默认 childrenInner 本就是 --spacing-6/24px),改动最小(唯一行为变更是一个 CSS 值 0→var(--spacing-6),其余是注释校正与测试)。!important 必要性判断准确(Astryx Stylex 类经 :not(##)×3 特异性达 (3,1,0),Maka (0,2,2) 不加 !important 会输);子组合器链只落到 childrenInner 不误伤更深嵌套。单测 + e2e 都在 CI 真实执行且全绿。P3-1:契约测试钉的是实现机制而非视觉结果——将来删掉这条像素零变化的多余 override 会被测试拦下;反向若 Astryx 改了自己的嵌套步进,!important 会让 Maka 静默停在 24px 且测试全绿,契约与设计系统悄悄脱节(建议改用 computed-style 断言)。P3-2:单测路径回退有死分支(desktop 构建里 dist/renderer/styles/sidebar.css 不存在,dist 运行态必落第二个 candidate 读源码)。P3-3:e2e 阈值 20 是魔法数,未指向 --spacing-6(24px) 容差;PR 描述称 Electron sidebar test 实为 Playwright spec。附注:时间排序贴左缘 vs 项目排序 24px inset 是明示取舍,属产品判断。

@Astro-Han
Astro-Han merged commit 5db971a into apache:mainAug 18, 2026
12 checks passed
@ARE404
ARE404 deleted the codex/fix-sidebar-session-indentation branch August 23, 2026 08:05
Sign up for freeto 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.

2 participants

@ARE404@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

fix(desktop): indent project session rows - #3175

Merged
Astro-Han merged 2 commits into
apache:mainfrom
ARE404:codex/fix-sidebar-session-indentation
Aug 18, 2026
Merged

fix(desktop): indent project session rows#3175
Astro-Han merged 2 commits into
apache:mainfrom
ARE404:codex/fix-sidebar-session-indentation

Conversation

@ARE404

@ARE404ARE404 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Restore the standard spacing-6 (24px) SideNav nesting step for sessions rendered under a project. The CSS contract test pins the hierarchy, and the Electron test verifies the visible indentation after waiting for both measured controls to become visible.

Verification

  • npx biome check apps/desktop/src/renderer/styles/sidebar.css packages/ui/src/session-history-list.tsx apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts apps/desktop/e2e/sidebar-project-row.spec.ts — passed
  • node --test apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts — 1 passed
  • npx playwright test --config e2e/playwright.config.ts e2e/sidebar-project-row.spec.ts — 3 passed
  • Desktop build and focused compiled contract tests passed before review

Review focus

The implementation restores the existing design-system spacing token rather than introducing a new value. The geometry assertion waits for the project and first-session controls to be visible before reading boundingBox(), addressing the review reliability concern.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex audited the sidebar hierarchy, implemented and tested the CSS/E2E fix, incorporated review feedback, and drafted this description. The human contributor reviewed the work, chose to submit it, and remains responsible for its accuracy, provenance, and licensing.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@coderabbitai

coderabbitaiBot commented Aug 17, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 93f15545-8c06-4318-bc6b-27b2be7308e2

📥 Commits

Reviewing files that changed from the base of the PR and between 5d9ce0d and e1906de.

📒 Files selected for processing (4)
  • apps/desktop/e2e/sidebar-project-row.spec.ts
  • apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts
  • apps/desktop/src/renderer/styles/sidebar.css
  • packages/ui/src/session-history-list.tsx

Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.


📝 Walkthrough

What this PR solves

Project session rows in the desktop sidebar lost their normal nesting indentation because the child container applied 0 !important. This PR restores the standard spacing-6 24px indentation. It also preserves selected-state inset and right-edge alignment.

Source of truth

The PR extends the existing sidebar CSS behavior. It does not create a parallel rendering path or public API. The contract test reads sidebar.css and verifies the required hierarchy rule.

Solution scope and complexity

The change is the smallest coherent solution:

  • Remove the 0 !important indentation override.
  • Add the required --spacing-6 offset.
  • Update the related component comment.
  • Add focused CSS contract and Electron geometry coverage.

The added tests are necessary to prevent both stylesheet regressions and visible layout regressions. No added implementation complexity appears unnecessary.

Simplification opportunities

No code or test can be removed without weakening either the CSS contract or the user-visible geometry coverage. The test setup could only be simplified if another existing test already provided equivalent coverage for both the rule and rendered spacing.

Risks and validation

The change affects desktop sidebar layout. Session rows now appear 24px farther from the project row. Incorrect CSS specificity could still prevent the intended rule from applying.

The PR adds:

  • A CSS contract test for the explicit 24px hierarchy rule.
  • An Electron test that waits for sidebar geometry and verifies at least 20px of horizontal separation.
  • Existing validation reported by the PR: desktop builds, unit tests, Playwright tests, Biome checks, and layout detection.

The final status of required checks is unverified from the available evidence.

Review-relevant risks

The current diff has a user-visible desktop layout effect. Material changes in user-visible behavior require independent human review under repository policy.

No public contract, security, licensing, release, or governance effect was identified in the current diff.

The person performing the merge must review the final diff. A maintainer makes the final determination.

Walkthrough

Project child session rows now render with a 24px nesting offset. Contract and end-to-end tests verify the stylesheet rule and horizontal separation.

Changes

Sidebar hierarchy

Layer / File(s)Summary
Project session indentation and validation
apps/desktop/src/renderer/styles/sidebar.css, packages/ui/src/session-history-list.tsx, apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts, apps/desktop/e2e/sidebar-project-row.spec.ts
Project child sessions use var(--spacing-6) inline-start padding. The related comment describes the nesting. Contract tests verify the CSS rule, and end-to-end tests verify visible horizontal separation.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk:⚪ Minimal · up to e1906

This PR restores project/session indentation and adds targeted regression coverage; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers:astro-han, jackwener

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Ai Use Disclosure⚠️ WarningThe PR description selects neither AI-use declaration, and both introduced commits have no Generated-by trailer.Select exactly one declaration and, if applicable, name the tool and scope. Follow CONTRIBUTING.md “Human ownership and AI attribution”; retain required trailers through squash or amend.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.
Description check✅ PassedThe description includes the required summary, verification, AI use, and checklist sections with specific implementation details and test results.
Title check✅ PassedThe title clearly and concisely describes the main change: restoring indentation for project session rows.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@ARE404
ARE404 marked this pull request as ready for review August 17, 2026 16:11
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix desktop sidebar: indent project session rows (restore spacing-6)

🐞 Bug fix🧪 Tests🕐 20-40 Minutes

Grey Divider

AI Description

• Restore Astryx SideNav child indentation (spacing-6 / 24px) for sessions under projects.
• Add a CSS contract test to pin the project→session hierarchy rule.
• Strengthen the Electron sidebar Playwright test with a geometry-based indentation assertion.
Diagram

graph TD
A["ProjectNavRow (ui)"] --> B["Electron renderer"] --> C["sidebar.css rule"]
D["CSS contract test"] --> C
E["Playwright sidebar e2e"] --> B
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Move indentation control into the component (StyleX)
  • ➕ Eliminates reliance on unlayered CSS overrides + !important
  • ➕ Keeps hierarchy styling co-located with the rendering logic
  • ➖ May require StyleX layering/ordering changes that are broader than this fix
  • ➖ Harder to validate across build outputs if CSS extraction differs by environment
2. Rely on Playwright geometry test only (drop CSS contract test)
  • ➕ Less brittle than regex-matching a CSS selector/body
  • ➕ Ensures the user-visible outcome is correct
  • ➖ Slower and potentially flakier than a pure unit/contract test
  • ➖ Harder to pinpoint regressions (CSS rule vs layout/DOM change)

Recommendation: Current approach (restore the design-system spacing token in sidebar.css, plus a fast CSS contract test and a user-visible geometry assertion) is the best tradeoff: it fixes the regression with minimal surface area, locks in the intended hierarchy at two levels (source CSS and rendered layout), and avoids introducing new spacing values.

Files changed (4) +44 / -8

Bug fix (1) +8 / -7
sidebar.cssRestore spacing-6 indentation for project child session container+8/-7

Restore spacing-6 indentation for project child session container

• Updates the .maka-project-row child-group rule to apply padding-inline-start: var(--spacing-6) !important instead of zeroing indentation. Rewrites the comment to reflect that project grouping is a true parent-child hierarchy and why the value is pinned.

apps/desktop/src/renderer/styles/sidebar.css

Tests (2) +35 / -0
sidebar-project-row.spec.tsAdd geometry assertion for project→session indentation+8/-0

Add geometry assertion for project→session indentation

• Captures bounding boxes for the project navigation control and the first session control, asserting the session is horizontally offset. This makes the indentation regression detectable in the Electron E2E suite rather than relying only on focus/order assertions.

apps/desktop/e2e/sidebar-project-row.spec.ts

session-project-hierarchy-contract.test.tsAdd contract test pinning sidebar CSS hierarchy rule+27/-0

Add contract test pinning sidebar CSS hierarchy rule

• Introduces a Node test that locates sidebar.css across source/dist paths and asserts the presence of a specific project-children selector. Pins the required padding-inline-start to var(--spacing-6) with !important as a visual contract.

apps/desktop/src/main/tests/session-project-hierarchy-contract.test.ts

Documentation (1) +1 / -1
session-history-list.tsxUpdate ProjectNavRow comment to match indentation behavior+1/-1

Update ProjectNavRow comment to match indentation behavior

• Adjusts the inline comment above the nested session rendering to reflect that sidebar.css preserves a standard SideNav nesting step for the project hierarchy.

packages/ui/src/session-history-list.tsx

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1)📘 Rule violations (0)📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Geometry assertion races visibility 🐞 Bug☼ Reliability
Description
The new test samples navigation.boundingBox() and firstSessionControl.boundingBox() immediately
after switching to project mode, without waiting for either element to be visible first.
boundingBox() does not auto-wait for visibility like assertion matchers do, so it can return
null while the sidebar is still re-rendering, making the new non-null assertions intermittently
fail in CI.
Code

apps/desktop/e2e/sidebar-project-row.spec.ts[R37-43]

+ const [projectNavigationBox, firstSessionBox] = await Promise.all([+ navigation.boundingBox(),+ firstSessionControl.boundingBox(),+ ]);+ expect(projectNavigationBox).not.toBeNull();+ expect(firstSessionBox).not.toBeNull();+ expect(firstSessionBox!.x - projectNavigationBox!.x).toBeGreaterThanOrEqual(20);
Relevance

●●● Strong

Matching accepted precedent explicitly requires visibility waits before boundingBox after UI
transitions.

PR-#3160

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The project-mode radio is clicked at line 20 and the DOM updates asynchronously
(aria-expanded/aria-controls wiring, session subtree mount). The new code at lines 37-43 immediately
calls boundingBox() on both locators and asserts non-null without first waiting for visibility
(e.g., via toBeVisible()), which is a documented gap since boundingBox() returns null for elements
not currently rendered/visible instead of waiting like other Playwright assertions.

PR-#3160

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
## Issue description
The newly added geometry assertion in the Playwright spec calls `boundingBox()` on `navigation` and `firstSessionControl` immediately after switching to project-grouped view, without waiting for either locator to be visible. `boundingBox()` returns `null` for elements that are not currently visible/rendered instead of auto-waiting, so this can produce a `null` value and fail the `not.toBeNull()` assertions intermittently under CI timing.
## Issue Context
The project row's collapsible session subtree is mounted/expanded asynchronously after clicking the '按项目' radio control. The test needs stable geometry data for both the project navigation control and the first session control before asserting the indentation.
## Fix Focus Areas
- apps/desktop/e2e/sidebar-project-row.spec.ts[37-43]
Add `await expect(navigation).toBeVisible()` and `await expect(firstSessionControl).toBeVisible()` before the `Promise.all` call that reads `boundingBox()` on both locators.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context
Review mode: 🚀 Fast: This is a localized, low-risk CSS hierarchy correction with focused contract and geometry tests, avoiding security, API, state, or cross-cutting architectural changes.

Grey Divider

Tip of the day
💡 Did you know, you can route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@ARE404
ARE404force-pushed the codex/fix-sidebar-session-indentation branch from e1906de to 2478d5aCompareAugust 18, 2026 03:05
@Astro-Han

Astro-Han commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Thanks — the fix is correct and minimal: the old padding-inline-start: 0 !important at sidebar.css:237-240 genuinely deviated from the design system (Astryx's default childrenInner is already --spacing-6/24px), and the change is exactly one CSS value plus comment correction and tests. The !important necessity check is accurate (Astryx Stylex classes carry (3,1,0) specificity via :not(#\#) — Maka's (0,2,2) loses without it), and the > div > [role="group"] > div combinator chain lands only on childrenInner without hitting deeper nesting. Both the new unit test and the e2e geometry assertion run in CI (both green). Time-sort rows keeping the left edge while project-sort rows inset 24px is a stated, documented choice — fine as a product decision.

Conclusion: PASS — all P3, nothing blocking.

P3-1 — the contract test pins the mechanism, not the visual result: if someone later deletes this now-redundant override (pixel-identical), the regex match fails and assert.ok(projectChildrenRule) goes red — the test would block a zero-pixel simplification. Conversely, if Astryx changes its own nesting step in the future, the !important override keeps Maka silently stuck at 24px with all tests green, and the contract drifts from the design system. A computed-style assertion would pin the visual outcome instead.

P3-2 — the unit test's candidate-path fallback has a dead branch: in the desktop build dist/renderer/styles/sidebar.css doesn't exist (renderer goes through vite bundle), so the dist run always falls to the second candidate reading the source; the first candidate implies an intent that doesn't exist. Dropping the dead candidate (or asserting against the vite artifact explicitly) would make the intent clear.

P3-3 — the e2e threshold toBeGreaterThanOrEqual(20) at e2e/sidebar-project-row.spec.ts:44 doesn't reference --spacing-6 (24px) with a tolerance note; it only guards against "zeroed", and a spacing-5 change would pass it. A one-line comment tying it to the token would help. Also, the PR body says "Electron sidebar test" but it's a Playwright spec — minor description nit.


AI-assisted review disclosure: this review was produced with AI assistance (pi review subagent on opencode-go/deepseek-v4-flash). The subagent verified specificity math, the descendant combinator chain, and CI execution paths statically; P3 items are static observations, not observed failures. Please weigh these findings with your own judgment.

中文摘要(AI 辅助审查)

结论:PASS,全 P3。问题真实(main 上 padding-inline-start: 0 !important 偏离设计系统,Astryx 默认 childrenInner 本就是 --spacing-6/24px),改动最小(唯一行为变更是一个 CSS 值 0→var(--spacing-6),其余是注释校正与测试)。!important 必要性判断准确(Astryx Stylex 类经 :not(##)×3 特异性达 (3,1,0),Maka (0,2,2) 不加 !important 会输);子组合器链只落到 childrenInner 不误伤更深嵌套。单测 + e2e 都在 CI 真实执行且全绿。P3-1:契约测试钉的是实现机制而非视觉结果——将来删掉这条像素零变化的多余 override 会被测试拦下;反向若 Astryx 改了自己的嵌套步进,!important 会让 Maka 静默停在 24px 且测试全绿,契约与设计系统悄悄脱节(建议改用 computed-style 断言)。P3-2:单测路径回退有死分支(desktop 构建里 dist/renderer/styles/sidebar.css 不存在,dist 运行态必落第二个 candidate 读源码)。P3-3:e2e 阈值 20 是魔法数,未指向 --spacing-6(24px) 容差;PR 描述称 Electron sidebar test 实为 Playwright spec。附注:时间排序贴左缘 vs 项目排序 24px inset 是明示取舍,属产品判断。

@Astro-Han
Astro-Han merged commit 5db971a into apache:mainAug 18, 2026
12 checks passed
@ARE404
ARE404 deleted the codex/fix-sidebar-session-indentation branch August 23, 2026 08:05
Sign up for freeto 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.

2 participants

@ARE404@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(desktop): indent project session rows - #3175

Merged
Astro-Han merged 2 commits into
apache:mainfrom
ARE404:codex/fix-sidebar-session-indentation
Aug 18, 2026
Merged

fix(desktop): indent project session rows#3175
Astro-Han merged 2 commits into
apache:mainfrom
ARE404:codex/fix-sidebar-session-indentation

Conversation

@ARE404

@ARE404ARE404 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Restore the standard spacing-6 (24px) SideNav nesting step for sessions rendered under a project. The CSS contract test pins the hierarchy, and the Electron test verifies the visible indentation after waiting for both measured controls to become visible.

Verification

  • npx biome check apps/desktop/src/renderer/styles/sidebar.css packages/ui/src/session-history-list.tsx apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts apps/desktop/e2e/sidebar-project-row.spec.ts — passed
  • node --test apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts — 1 passed
  • npx playwright test --config e2e/playwright.config.ts e2e/sidebar-project-row.spec.ts — 3 passed
  • Desktop build and focused compiled contract tests passed before review

Review focus

The implementation restores the existing design-system spacing token rather than introducing a new value. The geometry assertion waits for the project and first-session controls to be visible before reading boundingBox(), addressing the review reliability concern.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex audited the sidebar hierarchy, implemented and tested the CSS/E2E fix, incorporated review feedback, and drafted this description. The human contributor reviewed the work, chose to submit it, and remains responsible for its accuracy, provenance, and licensing.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@coderabbitai

coderabbitaiBot commented Aug 17, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 93f15545-8c06-4318-bc6b-27b2be7308e2

📥 Commits

Reviewing files that changed from the base of the PR and between 5d9ce0d and e1906de.

📒 Files selected for processing (4)
  • apps/desktop/e2e/sidebar-project-row.spec.ts
  • apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts
  • apps/desktop/src/renderer/styles/sidebar.css
  • packages/ui/src/session-history-list.tsx

Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.


📝 Walkthrough

What this PR solves

Project session rows in the desktop sidebar lost their normal nesting indentation because the child container applied 0 !important. This PR restores the standard spacing-6 24px indentation. It also preserves selected-state inset and right-edge alignment.

Source of truth

The PR extends the existing sidebar CSS behavior. It does not create a parallel rendering path or public API. The contract test reads sidebar.css and verifies the required hierarchy rule.

Solution scope and complexity

The change is the smallest coherent solution:

  • Remove the 0 !important indentation override.
  • Add the required --spacing-6 offset.
  • Update the related component comment.
  • Add focused CSS contract and Electron geometry coverage.

The added tests are necessary to prevent both stylesheet regressions and visible layout regressions. No added implementation complexity appears unnecessary.

Simplification opportunities

No code or test can be removed without weakening either the CSS contract or the user-visible geometry coverage. The test setup could only be simplified if another existing test already provided equivalent coverage for both the rule and rendered spacing.

Risks and validation

The change affects desktop sidebar layout. Session rows now appear 24px farther from the project row. Incorrect CSS specificity could still prevent the intended rule from applying.

The PR adds:

  • A CSS contract test for the explicit 24px hierarchy rule.
  • An Electron test that waits for sidebar geometry and verifies at least 20px of horizontal separation.
  • Existing validation reported by the PR: desktop builds, unit tests, Playwright tests, Biome checks, and layout detection.

The final status of required checks is unverified from the available evidence.

Review-relevant risks

The current diff has a user-visible desktop layout effect. Material changes in user-visible behavior require independent human review under repository policy.

No public contract, security, licensing, release, or governance effect was identified in the current diff.

The person performing the merge must review the final diff. A maintainer makes the final determination.

Walkthrough

Project child session rows now render with a 24px nesting offset. Contract and end-to-end tests verify the stylesheet rule and horizontal separation.

Changes

Sidebar hierarchy

Layer / File(s)Summary
Project session indentation and validation
apps/desktop/src/renderer/styles/sidebar.css, packages/ui/src/session-history-list.tsx, apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts, apps/desktop/e2e/sidebar-project-row.spec.ts
Project child sessions use var(--spacing-6) inline-start padding. The related comment describes the nesting. Contract tests verify the CSS rule, and end-to-end tests verify visible horizontal separation.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk:⚪ Minimal · up to e1906

This PR restores project/session indentation and adds targeted regression coverage; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers:astro-han, jackwener

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Ai Use Disclosure⚠️ WarningThe PR description selects neither AI-use declaration, and both introduced commits have no Generated-by trailer.Select exactly one declaration and, if applicable, name the tool and scope. Follow CONTRIBUTING.md “Human ownership and AI attribution”; retain required trailers through squash or amend.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.
Description check✅ PassedThe description includes the required summary, verification, AI use, and checklist sections with specific implementation details and test results.
Title check✅ PassedThe title clearly and concisely describes the main change: restoring indentation for project session rows.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@ARE404
ARE404 marked this pull request as ready for review August 17, 2026 16:11
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix desktop sidebar: indent project session rows (restore spacing-6)

🐞 Bug fix🧪 Tests🕐 20-40 Minutes

Grey Divider

AI Description

• Restore Astryx SideNav child indentation (spacing-6 / 24px) for sessions under projects.
• Add a CSS contract test to pin the project→session hierarchy rule.
• Strengthen the Electron sidebar Playwright test with a geometry-based indentation assertion.
Diagram

graph TD
A["ProjectNavRow (ui)"] --> B["Electron renderer"] --> C["sidebar.css rule"]
D["CSS contract test"] --> C
E["Playwright sidebar e2e"] --> B
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Move indentation control into the component (StyleX)
  • ➕ Eliminates reliance on unlayered CSS overrides + !important
  • ➕ Keeps hierarchy styling co-located with the rendering logic
  • ➖ May require StyleX layering/ordering changes that are broader than this fix
  • ➖ Harder to validate across build outputs if CSS extraction differs by environment
2. Rely on Playwright geometry test only (drop CSS contract test)
  • ➕ Less brittle than regex-matching a CSS selector/body
  • ➕ Ensures the user-visible outcome is correct
  • ➖ Slower and potentially flakier than a pure unit/contract test
  • ➖ Harder to pinpoint regressions (CSS rule vs layout/DOM change)

Recommendation: Current approach (restore the design-system spacing token in sidebar.css, plus a fast CSS contract test and a user-visible geometry assertion) is the best tradeoff: it fixes the regression with minimal surface area, locks in the intended hierarchy at two levels (source CSS and rendered layout), and avoids introducing new spacing values.

Files changed (4) +44 / -8

Bug fix (1) +8 / -7
sidebar.cssRestore spacing-6 indentation for project child session container+8/-7

Restore spacing-6 indentation for project child session container

• Updates the .maka-project-row child-group rule to apply padding-inline-start: var(--spacing-6) !important instead of zeroing indentation. Rewrites the comment to reflect that project grouping is a true parent-child hierarchy and why the value is pinned.

apps/desktop/src/renderer/styles/sidebar.css

Tests (2) +35 / -0
sidebar-project-row.spec.tsAdd geometry assertion for project→session indentation+8/-0

Add geometry assertion for project→session indentation

• Captures bounding boxes for the project navigation control and the first session control, asserting the session is horizontally offset. This makes the indentation regression detectable in the Electron E2E suite rather than relying only on focus/order assertions.

apps/desktop/e2e/sidebar-project-row.spec.ts

session-project-hierarchy-contract.test.tsAdd contract test pinning sidebar CSS hierarchy rule+27/-0

Add contract test pinning sidebar CSS hierarchy rule

• Introduces a Node test that locates sidebar.css across source/dist paths and asserts the presence of a specific project-children selector. Pins the required padding-inline-start to var(--spacing-6) with !important as a visual contract.

apps/desktop/src/main/tests/session-project-hierarchy-contract.test.ts

Documentation (1) +1 / -1
session-history-list.tsxUpdate ProjectNavRow comment to match indentation behavior+1/-1

Update ProjectNavRow comment to match indentation behavior

• Adjusts the inline comment above the nested session rendering to reflect that sidebar.css preserves a standard SideNav nesting step for the project hierarchy.

packages/ui/src/session-history-list.tsx

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1)📘 Rule violations (0)📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Geometry assertion races visibility 🐞 Bug☼ Reliability
Description
The new test samples navigation.boundingBox() and firstSessionControl.boundingBox() immediately
after switching to project mode, without waiting for either element to be visible first.
boundingBox() does not auto-wait for visibility like assertion matchers do, so it can return
null while the sidebar is still re-rendering, making the new non-null assertions intermittently
fail in CI.
Code

apps/desktop/e2e/sidebar-project-row.spec.ts[R37-43]

+ const [projectNavigationBox, firstSessionBox] = await Promise.all([+ navigation.boundingBox(),+ firstSessionControl.boundingBox(),+ ]);+ expect(projectNavigationBox).not.toBeNull();+ expect(firstSessionBox).not.toBeNull();+ expect(firstSessionBox!.x - projectNavigationBox!.x).toBeGreaterThanOrEqual(20);
Relevance

●●● Strong

Matching accepted precedent explicitly requires visibility waits before boundingBox after UI
transitions.

PR-#3160

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The project-mode radio is clicked at line 20 and the DOM updates asynchronously
(aria-expanded/aria-controls wiring, session subtree mount). The new code at lines 37-43 immediately
calls boundingBox() on both locators and asserts non-null without first waiting for visibility
(e.g., via toBeVisible()), which is a documented gap since boundingBox() returns null for elements
not currently rendered/visible instead of waiting like other Playwright assertions.

PR-#3160

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
## Issue description
The newly added geometry assertion in the Playwright spec calls `boundingBox()` on `navigation` and `firstSessionControl` immediately after switching to project-grouped view, without waiting for either locator to be visible. `boundingBox()` returns `null` for elements that are not currently visible/rendered instead of auto-waiting, so this can produce a `null` value and fail the `not.toBeNull()` assertions intermittently under CI timing.
## Issue Context
The project row's collapsible session subtree is mounted/expanded asynchronously after clicking the '按项目' radio control. The test needs stable geometry data for both the project navigation control and the first session control before asserting the indentation.
## Fix Focus Areas
- apps/desktop/e2e/sidebar-project-row.spec.ts[37-43]
Add `await expect(navigation).toBeVisible()` and `await expect(firstSessionControl).toBeVisible()` before the `Promise.all` call that reads `boundingBox()` on both locators.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context
Review mode: 🚀 Fast: This is a localized, low-risk CSS hierarchy correction with focused contract and geometry tests, avoiding security, API, state, or cross-cutting architectural changes.

Grey Divider

Tip of the day
💡 Did you know, you can route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@ARE404
ARE404force-pushed the codex/fix-sidebar-session-indentation branch from e1906de to 2478d5aCompareAugust 18, 2026 03:05
@Astro-Han

Astro-Han commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Thanks — the fix is correct and minimal: the old padding-inline-start: 0 !important at sidebar.css:237-240 genuinely deviated from the design system (Astryx's default childrenInner is already --spacing-6/24px), and the change is exactly one CSS value plus comment correction and tests. The !important necessity check is accurate (Astryx Stylex classes carry (3,1,0) specificity via :not(#\#) — Maka's (0,2,2) loses without it), and the > div > [role="group"] > div combinator chain lands only on childrenInner without hitting deeper nesting. Both the new unit test and the e2e geometry assertion run in CI (both green). Time-sort rows keeping the left edge while project-sort rows inset 24px is a stated, documented choice — fine as a product decision.

Conclusion: PASS — all P3, nothing blocking.

P3-1 — the contract test pins the mechanism, not the visual result: if someone later deletes this now-redundant override (pixel-identical), the regex match fails and assert.ok(projectChildrenRule) goes red — the test would block a zero-pixel simplification. Conversely, if Astryx changes its own nesting step in the future, the !important override keeps Maka silently stuck at 24px with all tests green, and the contract drifts from the design system. A computed-style assertion would pin the visual outcome instead.

P3-2 — the unit test's candidate-path fallback has a dead branch: in the desktop build dist/renderer/styles/sidebar.css doesn't exist (renderer goes through vite bundle), so the dist run always falls to the second candidate reading the source; the first candidate implies an intent that doesn't exist. Dropping the dead candidate (or asserting against the vite artifact explicitly) would make the intent clear.

P3-3 — the e2e threshold toBeGreaterThanOrEqual(20) at e2e/sidebar-project-row.spec.ts:44 doesn't reference --spacing-6 (24px) with a tolerance note; it only guards against "zeroed", and a spacing-5 change would pass it. A one-line comment tying it to the token would help. Also, the PR body says "Electron sidebar test" but it's a Playwright spec — minor description nit.


AI-assisted review disclosure: this review was produced with AI assistance (pi review subagent on opencode-go/deepseek-v4-flash). The subagent verified specificity math, the descendant combinator chain, and CI execution paths statically; P3 items are static observations, not observed failures. Please weigh these findings with your own judgment.

中文摘要(AI 辅助审查)

结论:PASS,全 P3。问题真实(main 上 padding-inline-start: 0 !important 偏离设计系统,Astryx 默认 childrenInner 本就是 --spacing-6/24px),改动最小(唯一行为变更是一个 CSS 值 0→var(--spacing-6),其余是注释校正与测试)。!important 必要性判断准确(Astryx Stylex 类经 :not(##)×3 特异性达 (3,1,0),Maka (0,2,2) 不加 !important 会输);子组合器链只落到 childrenInner 不误伤更深嵌套。单测 + e2e 都在 CI 真实执行且全绿。P3-1:契约测试钉的是实现机制而非视觉结果——将来删掉这条像素零变化的多余 override 会被测试拦下;反向若 Astryx 改了自己的嵌套步进,!important 会让 Maka 静默停在 24px 且测试全绿,契约与设计系统悄悄脱节(建议改用 computed-style 断言)。P3-2:单测路径回退有死分支(desktop 构建里 dist/renderer/styles/sidebar.css 不存在,dist 运行态必落第二个 candidate 读源码)。P3-3:e2e 阈值 20 是魔法数,未指向 --spacing-6(24px) 容差;PR 描述称 Electron sidebar test 实为 Playwright spec。附注:时间排序贴左缘 vs 项目排序 24px inset 是明示取舍,属产品判断。

@Astro-Han
Astro-Han merged commit 5db971a into apache:mainAug 18, 2026
12 checks passed
@ARE404
ARE404 deleted the codex/fix-sidebar-session-indentation branch August 23, 2026 08:05
Sign up for freeto 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.

2 participants

@ARE404@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(desktop): indent project session rows - #3175

Merged
Astro-Han merged 2 commits into
apache:mainfrom
ARE404:codex/fix-sidebar-session-indentation
Aug 18, 2026
Merged

fix(desktop): indent project session rows#3175
Astro-Han merged 2 commits into
apache:mainfrom
ARE404:codex/fix-sidebar-session-indentation

Conversation

@ARE404

@ARE404ARE404 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Restore the standard spacing-6 (24px) SideNav nesting step for sessions rendered under a project. The CSS contract test pins the hierarchy, and the Electron test verifies the visible indentation after waiting for both measured controls to become visible.

Verification

  • npx biome check apps/desktop/src/renderer/styles/sidebar.css packages/ui/src/session-history-list.tsx apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts apps/desktop/e2e/sidebar-project-row.spec.ts — passed
  • node --test apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts — 1 passed
  • npx playwright test --config e2e/playwright.config.ts e2e/sidebar-project-row.spec.ts — 3 passed
  • Desktop build and focused compiled contract tests passed before review

Review focus

The implementation restores the existing design-system spacing token rather than introducing a new value. The geometry assertion waits for the project and first-session controls to be visible before reading boundingBox(), addressing the review reliability concern.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex audited the sidebar hierarchy, implemented and tested the CSS/E2E fix, incorporated review feedback, and drafted this description. The human contributor reviewed the work, chose to submit it, and remains responsible for its accuracy, provenance, and licensing.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@coderabbitai

coderabbitaiBot commented Aug 17, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 93f15545-8c06-4318-bc6b-27b2be7308e2

📥 Commits

Reviewing files that changed from the base of the PR and between 5d9ce0d and e1906de.

📒 Files selected for processing (4)
  • apps/desktop/e2e/sidebar-project-row.spec.ts
  • apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts
  • apps/desktop/src/renderer/styles/sidebar.css
  • packages/ui/src/session-history-list.tsx

Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.


📝 Walkthrough

What this PR solves

Project session rows in the desktop sidebar lost their normal nesting indentation because the child container applied 0 !important. This PR restores the standard spacing-6 24px indentation. It also preserves selected-state inset and right-edge alignment.

Source of truth

The PR extends the existing sidebar CSS behavior. It does not create a parallel rendering path or public API. The contract test reads sidebar.css and verifies the required hierarchy rule.

Solution scope and complexity

The change is the smallest coherent solution:

  • Remove the 0 !important indentation override.
  • Add the required --spacing-6 offset.
  • Update the related component comment.
  • Add focused CSS contract and Electron geometry coverage.

The added tests are necessary to prevent both stylesheet regressions and visible layout regressions. No added implementation complexity appears unnecessary.

Simplification opportunities

No code or test can be removed without weakening either the CSS contract or the user-visible geometry coverage. The test setup could only be simplified if another existing test already provided equivalent coverage for both the rule and rendered spacing.

Risks and validation

The change affects desktop sidebar layout. Session rows now appear 24px farther from the project row. Incorrect CSS specificity could still prevent the intended rule from applying.

The PR adds:

  • A CSS contract test for the explicit 24px hierarchy rule.
  • An Electron test that waits for sidebar geometry and verifies at least 20px of horizontal separation.
  • Existing validation reported by the PR: desktop builds, unit tests, Playwright tests, Biome checks, and layout detection.

The final status of required checks is unverified from the available evidence.

Review-relevant risks

The current diff has a user-visible desktop layout effect. Material changes in user-visible behavior require independent human review under repository policy.

No public contract, security, licensing, release, or governance effect was identified in the current diff.

The person performing the merge must review the final diff. A maintainer makes the final determination.

Walkthrough

Project child session rows now render with a 24px nesting offset. Contract and end-to-end tests verify the stylesheet rule and horizontal separation.

Changes

Sidebar hierarchy

Layer / File(s)Summary
Project session indentation and validation
apps/desktop/src/renderer/styles/sidebar.css, packages/ui/src/session-history-list.tsx, apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts, apps/desktop/e2e/sidebar-project-row.spec.ts
Project child sessions use var(--spacing-6) inline-start padding. The related comment describes the nesting. Contract tests verify the CSS rule, and end-to-end tests verify visible horizontal separation.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk:⚪ Minimal · up to e1906

This PR restores project/session indentation and adds targeted regression coverage; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers:astro-han, jackwener

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Ai Use Disclosure⚠️ WarningThe PR description selects neither AI-use declaration, and both introduced commits have no Generated-by trailer.Select exactly one declaration and, if applicable, name the tool and scope. Follow CONTRIBUTING.md “Human ownership and AI attribution”; retain required trailers through squash or amend.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.
Description check✅ PassedThe description includes the required summary, verification, AI use, and checklist sections with specific implementation details and test results.
Title check✅ PassedThe title clearly and concisely describes the main change: restoring indentation for project session rows.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@ARE404
ARE404 marked this pull request as ready for review August 17, 2026 16:11
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix desktop sidebar: indent project session rows (restore spacing-6)

🐞 Bug fix🧪 Tests🕐 20-40 Minutes

Grey Divider

AI Description

• Restore Astryx SideNav child indentation (spacing-6 / 24px) for sessions under projects.
• Add a CSS contract test to pin the project→session hierarchy rule.
• Strengthen the Electron sidebar Playwright test with a geometry-based indentation assertion.
Diagram

graph TD
A["ProjectNavRow (ui)"] --> B["Electron renderer"] --> C["sidebar.css rule"]
D["CSS contract test"] --> C
E["Playwright sidebar e2e"] --> B
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Move indentation control into the component (StyleX)
  • ➕ Eliminates reliance on unlayered CSS overrides + !important
  • ➕ Keeps hierarchy styling co-located with the rendering logic
  • ➖ May require StyleX layering/ordering changes that are broader than this fix
  • ➖ Harder to validate across build outputs if CSS extraction differs by environment
2. Rely on Playwright geometry test only (drop CSS contract test)
  • ➕ Less brittle than regex-matching a CSS selector/body
  • ➕ Ensures the user-visible outcome is correct
  • ➖ Slower and potentially flakier than a pure unit/contract test
  • ➖ Harder to pinpoint regressions (CSS rule vs layout/DOM change)

Recommendation: Current approach (restore the design-system spacing token in sidebar.css, plus a fast CSS contract test and a user-visible geometry assertion) is the best tradeoff: it fixes the regression with minimal surface area, locks in the intended hierarchy at two levels (source CSS and rendered layout), and avoids introducing new spacing values.

Files changed (4) +44 / -8

Bug fix (1) +8 / -7
sidebar.cssRestore spacing-6 indentation for project child session container+8/-7

Restore spacing-6 indentation for project child session container

• Updates the .maka-project-row child-group rule to apply padding-inline-start: var(--spacing-6) !important instead of zeroing indentation. Rewrites the comment to reflect that project grouping is a true parent-child hierarchy and why the value is pinned.

apps/desktop/src/renderer/styles/sidebar.css

Tests (2) +35 / -0
sidebar-project-row.spec.tsAdd geometry assertion for project→session indentation+8/-0

Add geometry assertion for project→session indentation

• Captures bounding boxes for the project navigation control and the first session control, asserting the session is horizontally offset. This makes the indentation regression detectable in the Electron E2E suite rather than relying only on focus/order assertions.

apps/desktop/e2e/sidebar-project-row.spec.ts

session-project-hierarchy-contract.test.tsAdd contract test pinning sidebar CSS hierarchy rule+27/-0

Add contract test pinning sidebar CSS hierarchy rule

• Introduces a Node test that locates sidebar.css across source/dist paths and asserts the presence of a specific project-children selector. Pins the required padding-inline-start to var(--spacing-6) with !important as a visual contract.

apps/desktop/src/main/tests/session-project-hierarchy-contract.test.ts

Documentation (1) +1 / -1
session-history-list.tsxUpdate ProjectNavRow comment to match indentation behavior+1/-1

Update ProjectNavRow comment to match indentation behavior

• Adjusts the inline comment above the nested session rendering to reflect that sidebar.css preserves a standard SideNav nesting step for the project hierarchy.

packages/ui/src/session-history-list.tsx

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1)📘 Rule violations (0)📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Geometry assertion races visibility 🐞 Bug☼ Reliability
Description
The new test samples navigation.boundingBox() and firstSessionControl.boundingBox() immediately
after switching to project mode, without waiting for either element to be visible first.
boundingBox() does not auto-wait for visibility like assertion matchers do, so it can return
null while the sidebar is still re-rendering, making the new non-null assertions intermittently
fail in CI.
Code

apps/desktop/e2e/sidebar-project-row.spec.ts[R37-43]

+ const [projectNavigationBox, firstSessionBox] = await Promise.all([+ navigation.boundingBox(),+ firstSessionControl.boundingBox(),+ ]);+ expect(projectNavigationBox).not.toBeNull();+ expect(firstSessionBox).not.toBeNull();+ expect(firstSessionBox!.x - projectNavigationBox!.x).toBeGreaterThanOrEqual(20);
Relevance

●●● Strong

Matching accepted precedent explicitly requires visibility waits before boundingBox after UI
transitions.

PR-#3160

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The project-mode radio is clicked at line 20 and the DOM updates asynchronously
(aria-expanded/aria-controls wiring, session subtree mount). The new code at lines 37-43 immediately
calls boundingBox() on both locators and asserts non-null without first waiting for visibility
(e.g., via toBeVisible()), which is a documented gap since boundingBox() returns null for elements
not currently rendered/visible instead of waiting like other Playwright assertions.

PR-#3160

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
## Issue description
The newly added geometry assertion in the Playwright spec calls `boundingBox()` on `navigation` and `firstSessionControl` immediately after switching to project-grouped view, without waiting for either locator to be visible. `boundingBox()` returns `null` for elements that are not currently visible/rendered instead of auto-waiting, so this can produce a `null` value and fail the `not.toBeNull()` assertions intermittently under CI timing.
## Issue Context
The project row's collapsible session subtree is mounted/expanded asynchronously after clicking the '按项目' radio control. The test needs stable geometry data for both the project navigation control and the first session control before asserting the indentation.
## Fix Focus Areas
- apps/desktop/e2e/sidebar-project-row.spec.ts[37-43]
Add `await expect(navigation).toBeVisible()` and `await expect(firstSessionControl).toBeVisible()` before the `Promise.all` call that reads `boundingBox()` on both locators.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context
Review mode: 🚀 Fast: This is a localized, low-risk CSS hierarchy correction with focused contract and geometry tests, avoiding security, API, state, or cross-cutting architectural changes.

Grey Divider

Tip of the day
💡 Did you know, you can route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@ARE404
ARE404force-pushed the codex/fix-sidebar-session-indentation branch from e1906de to 2478d5aCompareAugust 18, 2026 03:05
@Astro-Han

Astro-Han commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Thanks — the fix is correct and minimal: the old padding-inline-start: 0 !important at sidebar.css:237-240 genuinely deviated from the design system (Astryx's default childrenInner is already --spacing-6/24px), and the change is exactly one CSS value plus comment correction and tests. The !important necessity check is accurate (Astryx Stylex classes carry (3,1,0) specificity via :not(#\#) — Maka's (0,2,2) loses without it), and the > div > [role="group"] > div combinator chain lands only on childrenInner without hitting deeper nesting. Both the new unit test and the e2e geometry assertion run in CI (both green). Time-sort rows keeping the left edge while project-sort rows inset 24px is a stated, documented choice — fine as a product decision.

Conclusion: PASS — all P3, nothing blocking.

P3-1 — the contract test pins the mechanism, not the visual result: if someone later deletes this now-redundant override (pixel-identical), the regex match fails and assert.ok(projectChildrenRule) goes red — the test would block a zero-pixel simplification. Conversely, if Astryx changes its own nesting step in the future, the !important override keeps Maka silently stuck at 24px with all tests green, and the contract drifts from the design system. A computed-style assertion would pin the visual outcome instead.

P3-2 — the unit test's candidate-path fallback has a dead branch: in the desktop build dist/renderer/styles/sidebar.css doesn't exist (renderer goes through vite bundle), so the dist run always falls to the second candidate reading the source; the first candidate implies an intent that doesn't exist. Dropping the dead candidate (or asserting against the vite artifact explicitly) would make the intent clear.

P3-3 — the e2e threshold toBeGreaterThanOrEqual(20) at e2e/sidebar-project-row.spec.ts:44 doesn't reference --spacing-6 (24px) with a tolerance note; it only guards against "zeroed", and a spacing-5 change would pass it. A one-line comment tying it to the token would help. Also, the PR body says "Electron sidebar test" but it's a Playwright spec — minor description nit.


AI-assisted review disclosure: this review was produced with AI assistance (pi review subagent on opencode-go/deepseek-v4-flash). The subagent verified specificity math, the descendant combinator chain, and CI execution paths statically; P3 items are static observations, not observed failures. Please weigh these findings with your own judgment.

中文摘要(AI 辅助审查)

结论:PASS,全 P3。问题真实(main 上 padding-inline-start: 0 !important 偏离设计系统,Astryx 默认 childrenInner 本就是 --spacing-6/24px),改动最小(唯一行为变更是一个 CSS 值 0→var(--spacing-6),其余是注释校正与测试)。!important 必要性判断准确(Astryx Stylex 类经 :not(##)×3 特异性达 (3,1,0),Maka (0,2,2) 不加 !important 会输);子组合器链只落到 childrenInner 不误伤更深嵌套。单测 + e2e 都在 CI 真实执行且全绿。P3-1:契约测试钉的是实现机制而非视觉结果——将来删掉这条像素零变化的多余 override 会被测试拦下;反向若 Astryx 改了自己的嵌套步进,!important 会让 Maka 静默停在 24px 且测试全绿,契约与设计系统悄悄脱节(建议改用 computed-style 断言)。P3-2:单测路径回退有死分支(desktop 构建里 dist/renderer/styles/sidebar.css 不存在,dist 运行态必落第二个 candidate 读源码)。P3-3:e2e 阈值 20 是魔法数,未指向 --spacing-6(24px) 容差;PR 描述称 Electron sidebar test 实为 Playwright spec。附注:时间排序贴左缘 vs 项目排序 24px inset 是明示取舍,属产品判断。

@Astro-Han
Astro-Han merged commit 5db971a into apache:mainAug 18, 2026
12 checks passed
@ARE404
ARE404 deleted the codex/fix-sidebar-session-indentation branch August 23, 2026 08:05
Sign up for freeto 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.

2 participants

@ARE404@Astro-Han
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

fix(desktop): indent project session rows - #3175

Merged
Astro-Han merged 2 commits into
apache:mainfrom
ARE404:codex/fix-sidebar-session-indentation
Aug 18, 2026
Merged

fix(desktop): indent project session rows#3175
Astro-Han merged 2 commits into
apache:mainfrom
ARE404:codex/fix-sidebar-session-indentation

Conversation

@ARE404

@ARE404ARE404 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Restore the standard spacing-6 (24px) SideNav nesting step for sessions rendered under a project. The CSS contract test pins the hierarchy, and the Electron test verifies the visible indentation after waiting for both measured controls to become visible.

Verification

  • npx biome check apps/desktop/src/renderer/styles/sidebar.css packages/ui/src/session-history-list.tsx apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts apps/desktop/e2e/sidebar-project-row.spec.ts — passed
  • node --test apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts — 1 passed
  • npx playwright test --config e2e/playwright.config.ts e2e/sidebar-project-row.spec.ts — 3 passed
  • Desktop build and focused compiled contract tests passed before review

Review focus

The implementation restores the existing design-system spacing token rather than introducing a new value. The geometry assertion waits for the project and first-session controls to be visible before reading boundingBox(), addressing the review reliability concern.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex audited the sidebar hierarchy, implemented and tested the CSS/E2E fix, incorporated review feedback, and drafted this description. The human contributor reviewed the work, chose to submit it, and remains responsible for its accuracy, provenance, and licensing.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@coderabbitai

coderabbitaiBot commented Aug 17, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 93f15545-8c06-4318-bc6b-27b2be7308e2

📥 Commits

Reviewing files that changed from the base of the PR and between 5d9ce0d and e1906de.

📒 Files selected for processing (4)
  • apps/desktop/e2e/sidebar-project-row.spec.ts
  • apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts
  • apps/desktop/src/renderer/styles/sidebar.css
  • packages/ui/src/session-history-list.tsx

Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.


📝 Walkthrough

What this PR solves

Project session rows in the desktop sidebar lost their normal nesting indentation because the child container applied 0 !important. This PR restores the standard spacing-6 24px indentation. It also preserves selected-state inset and right-edge alignment.

Source of truth

The PR extends the existing sidebar CSS behavior. It does not create a parallel rendering path or public API. The contract test reads sidebar.css and verifies the required hierarchy rule.

Solution scope and complexity

The change is the smallest coherent solution:

  • Remove the 0 !important indentation override.
  • Add the required --spacing-6 offset.
  • Update the related component comment.
  • Add focused CSS contract and Electron geometry coverage.

The added tests are necessary to prevent both stylesheet regressions and visible layout regressions. No added implementation complexity appears unnecessary.

Simplification opportunities

No code or test can be removed without weakening either the CSS contract or the user-visible geometry coverage. The test setup could only be simplified if another existing test already provided equivalent coverage for both the rule and rendered spacing.

Risks and validation

The change affects desktop sidebar layout. Session rows now appear 24px farther from the project row. Incorrect CSS specificity could still prevent the intended rule from applying.

The PR adds:

  • A CSS contract test for the explicit 24px hierarchy rule.
  • An Electron test that waits for sidebar geometry and verifies at least 20px of horizontal separation.
  • Existing validation reported by the PR: desktop builds, unit tests, Playwright tests, Biome checks, and layout detection.

The final status of required checks is unverified from the available evidence.

Review-relevant risks

The current diff has a user-visible desktop layout effect. Material changes in user-visible behavior require independent human review under repository policy.

No public contract, security, licensing, release, or governance effect was identified in the current diff.

The person performing the merge must review the final diff. A maintainer makes the final determination.

Walkthrough

Project child session rows now render with a 24px nesting offset. Contract and end-to-end tests verify the stylesheet rule and horizontal separation.

Changes

Sidebar hierarchy

Layer / File(s)Summary
Project session indentation and validation
apps/desktop/src/renderer/styles/sidebar.css, packages/ui/src/session-history-list.tsx, apps/desktop/src/main/__tests__/session-project-hierarchy-contract.test.ts, apps/desktop/e2e/sidebar-project-row.spec.ts
Project child sessions use var(--spacing-6) inline-start padding. The related comment describes the nesting. Contract tests verify the CSS rule, and end-to-end tests verify visible horizontal separation.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk:⚪ Minimal · up to e1906

This PR restores project/session indentation and adds targeted regression coverage; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers:astro-han, jackwener

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Ai Use Disclosure⚠️ WarningThe PR description selects neither AI-use declaration, and both introduced commits have no Generated-by trailer.Select exactly one declaration and, if applicable, name the tool and scope. Follow CONTRIBUTING.md “Human ownership and AI attribution”; retain required trailers through squash or amend.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.
Description check✅ PassedThe description includes the required summary, verification, AI use, and checklist sections with specific implementation details and test results.
Title check✅ PassedThe title clearly and concisely describes the main change: restoring indentation for project session rows.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@ARE404
ARE404 marked this pull request as ready for review August 17, 2026 16:11
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix desktop sidebar: indent project session rows (restore spacing-6)

🐞 Bug fix🧪 Tests🕐 20-40 Minutes

Grey Divider

AI Description

• Restore Astryx SideNav child indentation (spacing-6 / 24px) for sessions under projects.
• Add a CSS contract test to pin the project→session hierarchy rule.
• Strengthen the Electron sidebar Playwright test with a geometry-based indentation assertion.
Diagram

graph TD
A["ProjectNavRow (ui)"] --> B["Electron renderer"] --> C["sidebar.css rule"]
D["CSS contract test"] --> C
E["Playwright sidebar e2e"] --> B
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Move indentation control into the component (StyleX)
  • ➕ Eliminates reliance on unlayered CSS overrides + !important
  • ➕ Keeps hierarchy styling co-located with the rendering logic
  • ➖ May require StyleX layering/ordering changes that are broader than this fix
  • ➖ Harder to validate across build outputs if CSS extraction differs by environment
2. Rely on Playwright geometry test only (drop CSS contract test)
  • ➕ Less brittle than regex-matching a CSS selector/body
  • ➕ Ensures the user-visible outcome is correct
  • ➖ Slower and potentially flakier than a pure unit/contract test
  • ➖ Harder to pinpoint regressions (CSS rule vs layout/DOM change)

Recommendation: Current approach (restore the design-system spacing token in sidebar.css, plus a fast CSS contract test and a user-visible geometry assertion) is the best tradeoff: it fixes the regression with minimal surface area, locks in the intended hierarchy at two levels (source CSS and rendered layout), and avoids introducing new spacing values.

Files changed (4) +44 / -8

Bug fix (1) +8 / -7
sidebar.cssRestore spacing-6 indentation for project child session container+8/-7

Restore spacing-6 indentation for project child session container

• Updates the .maka-project-row child-group rule to apply padding-inline-start: var(--spacing-6) !important instead of zeroing indentation. Rewrites the comment to reflect that project grouping is a true parent-child hierarchy and why the value is pinned.

apps/desktop/src/renderer/styles/sidebar.css

Tests (2) +35 / -0
sidebar-project-row.spec.tsAdd geometry assertion for project→session indentation+8/-0

Add geometry assertion for project→session indentation

• Captures bounding boxes for the project navigation control and the first session control, asserting the session is horizontally offset. This makes the indentation regression detectable in the Electron E2E suite rather than relying only on focus/order assertions.

apps/desktop/e2e/sidebar-project-row.spec.ts

session-project-hierarchy-contract.test.tsAdd contract test pinning sidebar CSS hierarchy rule+27/-0

Add contract test pinning sidebar CSS hierarchy rule

• Introduces a Node test that locates sidebar.css across source/dist paths and asserts the presence of a specific project-children selector. Pins the required padding-inline-start to var(--spacing-6) with !important as a visual contract.

apps/desktop/src/main/tests/session-project-hierarchy-contract.test.ts

Documentation (1) +1 / -1
session-history-list.tsxUpdate ProjectNavRow comment to match indentation behavior+1/-1

Update ProjectNavRow comment to match indentation behavior

• Adjusts the inline comment above the nested session rendering to reflect that sidebar.css preserves a standard SideNav nesting step for the project hierarchy.

packages/ui/src/session-history-list.tsx

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1)📘 Rule violations (0)📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Geometry assertion races visibility 🐞 Bug☼ Reliability
Description
The new test samples navigation.boundingBox() and firstSessionControl.boundingBox() immediately
after switching to project mode, without waiting for either element to be visible first.
boundingBox() does not auto-wait for visibility like assertion matchers do, so it can return
null while the sidebar is still re-rendering, making the new non-null assertions intermittently
fail in CI.
Code

apps/desktop/e2e/sidebar-project-row.spec.ts[R37-43]

+ const [projectNavigationBox, firstSessionBox] = await Promise.all([+ navigation.boundingBox(),+ firstSessionControl.boundingBox(),+ ]);+ expect(projectNavigationBox).not.toBeNull();+ expect(firstSessionBox).not.toBeNull();+ expect(firstSessionBox!.x - projectNavigationBox!.x).toBeGreaterThanOrEqual(20);
Relevance

●●● Strong

Matching accepted precedent explicitly requires visibility waits before boundingBox after UI
transitions.

PR-#3160

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The project-mode radio is clicked at line 20 and the DOM updates asynchronously
(aria-expanded/aria-controls wiring, session subtree mount). The new code at lines 37-43 immediately
calls boundingBox() on both locators and asserts non-null without first waiting for visibility
(e.g., via toBeVisible()), which is a documented gap since boundingBox() returns null for elements
not currently rendered/visible instead of waiting like other Playwright assertions.

PR-#3160

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
## Issue description
The newly added geometry assertion in the Playwright spec calls `boundingBox()` on `navigation` and `firstSessionControl` immediately after switching to project-grouped view, without waiting for either locator to be visible. `boundingBox()` returns `null` for elements that are not currently visible/rendered instead of auto-waiting, so this can produce a `null` value and fail the `not.toBeNull()` assertions intermittently under CI timing.
## Issue Context
The project row's collapsible session subtree is mounted/expanded asynchronously after clicking the '按项目' radio control. The test needs stable geometry data for both the project navigation control and the first session control before asserting the indentation.
## Fix Focus Areas
- apps/desktop/e2e/sidebar-project-row.spec.ts[37-43]
Add `await expect(navigation).toBeVisible()` and `await expect(firstSessionControl).toBeVisible()` before the `Promise.all` call that reads `boundingBox()` on both locators.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context
Review mode: 🚀 Fast: This is a localized, low-risk CSS hierarchy correction with focused contract and geometry tests, avoiding security, API, state, or cross-cutting architectural changes.

Grey Divider

Tip of the day
💡 Did you know, you can route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@ARE404
ARE404force-pushed the codex/fix-sidebar-session-indentation branch from e1906de to 2478d5aCompareAugust 18, 2026 03:05
@Astro-Han

Astro-Han commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Thanks — the fix is correct and minimal: the old padding-inline-start: 0 !important at sidebar.css:237-240 genuinely deviated from the design system (Astryx's default childrenInner is already --spacing-6/24px), and the change is exactly one CSS value plus comment correction and tests. The !important necessity check is accurate (Astryx Stylex classes carry (3,1,0) specificity via :not(#\#) — Maka's (0,2,2) loses without it), and the > div > [role="group"] > div combinator chain lands only on childrenInner without hitting deeper nesting. Both the new unit test and the e2e geometry assertion run in CI (both green). Time-sort rows keeping the left edge while project-sort rows inset 24px is a stated, documented choice — fine as a product decision.

Conclusion: PASS — all P3, nothing blocking.

P3-1 — the contract test pins the mechanism, not the visual result: if someone later deletes this now-redundant override (pixel-identical), the regex match fails and assert.ok(projectChildrenRule) goes red — the test would block a zero-pixel simplification. Conversely, if Astryx changes its own nesting step in the future, the !important override keeps Maka silently stuck at 24px with all tests green, and the contract drifts from the design system. A computed-style assertion would pin the visual outcome instead.

P3-2 — the unit test's candidate-path fallback has a dead branch: in the desktop build dist/renderer/styles/sidebar.css doesn't exist (renderer goes through vite bundle), so the dist run always falls to the second candidate reading the source; the first candidate implies an intent that doesn't exist. Dropping the dead candidate (or asserting against the vite artifact explicitly) would make the intent clear.

P3-3 — the e2e threshold toBeGreaterThanOrEqual(20) at e2e/sidebar-project-row.spec.ts:44 doesn't reference --spacing-6 (24px) with a tolerance note; it only guards against "zeroed", and a spacing-5 change would pass it. A one-line comment tying it to the token would help. Also, the PR body says "Electron sidebar test" but it's a Playwright spec — minor description nit.


AI-assisted review disclosure: this review was produced with AI assistance (pi review subagent on opencode-go/deepseek-v4-flash). The subagent verified specificity math, the descendant combinator chain, and CI execution paths statically; P3 items are static observations, not observed failures. Please weigh these findings with your own judgment.

中文摘要(AI 辅助审查)

结论:PASS,全 P3。问题真实(main 上 padding-inline-start: 0 !important 偏离设计系统,Astryx 默认 childrenInner 本就是 --spacing-6/24px),改动最小(唯一行为变更是一个 CSS 值 0→var(--spacing-6),其余是注释校正与测试)。!important 必要性判断准确(Astryx Stylex 类经 :not(##)×3 特异性达 (3,1,0),Maka (0,2,2) 不加 !important 会输);子组合器链只落到 childrenInner 不误伤更深嵌套。单测 + e2e 都在 CI 真实执行且全绿。P3-1:契约测试钉的是实现机制而非视觉结果——将来删掉这条像素零变化的多余 override 会被测试拦下;反向若 Astryx 改了自己的嵌套步进,!important 会让 Maka 静默停在 24px 且测试全绿,契约与设计系统悄悄脱节(建议改用 computed-style 断言)。P3-2:单测路径回退有死分支(desktop 构建里 dist/renderer/styles/sidebar.css 不存在,dist 运行态必落第二个 candidate 读源码)。P3-3:e2e 阈值 20 是魔法数,未指向 --spacing-6(24px) 容差;PR 描述称 Electron sidebar test 实为 Playwright spec。附注:时间排序贴左缘 vs 项目排序 24px inset 是明示取舍,属产品判断。

@Astro-Han
Astro-Han merged commit 5db971a into apache:mainAug 18, 2026
12 checks passed
@ARE404
ARE404 deleted the codex/fix-sidebar-session-indentation branch August 23, 2026 08:05
Sign up for freeto 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.

2 participants

@ARE404@Astro-Han