Skip to content

feat(reasonix): native-ui polish — session sharing, shutdown, UI fixes + CI/test (issues #43-#49, #56) - #58

Merged
linletian merged 4 commits into
developfrom
feature/reasonix-native-ui
Aug 12, 2026
Merged

feat(reasonix): native-ui polish — session sharing, shutdown, UI fixes + CI/test (issues #43-#49, #56)#58
linletian merged 4 commits into
developfrom
feature/reasonix-native-ui

Conversation

@linletian

Copy link
Copy Markdown
Owner

概述

feature/reasonix-native-uidevelop 的 PR,包含 native-ui 的完善与修复。

主要变更

  • 会话池共享(issue bug(reasonix): 侧边栏历史 session 记录时有时无,导致无法切换历史会话 #56):去掉 per-instance REASONIX_HOME 隔离,共享 session pool
  • 嵌入 Web UI 保活:切换 tab 时保持 embedded web UI 存活,修复 iframe 刷新抖动
  • 关闭清理:myworktree shutdown 时停止 reasonix 实例
  • 网络监听回退:主 listener 为 network-open 时回退到同源 /rx/
  • UI 细节:折叠侧栏时固定 transcript/footer 网格槽位、调整 sidebar toggle 尺寸
  • 审查反馈(PR review):query pass-through、per-instance Start lock、encoding negotiation
  • 测试与 CI:新增 opt-in upstream contract check(脚本 + 测试 + CI);CI 安装 zsh 并跳过缺失时的 preStart 测试;gofmt 3 个文件

说明

当前分支相对 develop 落后 1 个提交(仅 PR #50 的 merge commit,内容已包含在本分支历史中),如需可通过 GitHub "Update branch" 或 gh pr update-branch 同步。

The injected hide/expand sidebar button in the embedded Reasonix web chat
was 34x34px at (8,8), so its right side overlapped the conversation. It is
now a vertical 24x64px pill at (2,8) showing a CSS arrow glyph that
switches with state (▶ collapsed / ◀ expanded, pointing at where the
sidebar moves — no text, no i18n). Measured against the upstream
.transcript padding (24px 28px on desktop), the button's right edge (26px)
stays inside the chat's 28px left gutter, so it never covers message text
in either state; the taller target is easier to see and click.

Layout tests now use grep-style per-property regexp assertions instead of
tight substring matches, so CSS property reordering no longer breaks them.

@linletian linletian left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR#58 评审(中文白话)

范围说明:本 PR 的 base 是 c592473(PR #57 的 merge commit),gh pr diff 实际只包含 2 个提交、3 个文件:CHANGELOG.mdinternal/app/reasonix_proxy.gointernal/app/reasonix_test.go。本次评审只针对 PR#58 的这 3 个文件。

整体评价

PR#58 是个小而聚焦的"补完打磨":把嵌入 reasonix 聊天里的侧栏切换按钮从 34×34 的方块改成 24×64 的细长 pill(塞进上游 .transcript 的 28px 左 padding 里不挡文字),用 CSS ::before 的 ▶/◀ 箭头取代 JS textContent='☰'(顺带解决 i18n 顾虑),测试改成"每条 CSS 属性一条 regex 断言"、不再因属性顺序假阳。

优点

  • 尺寸测量有理有据(注释里 2+24=26px ≤ 28px gutter),并明确写了"上游 .transcript padding 改了要重验"的看护条件。
  • 箭头方向 ▶/◀ 表示"侧栏移动方向"(点 ▶ 展开到右边、点 ◀ 收回左边),比 ☰ 更直观,且零文本零 i18n。
  • b.textContent 删了,测试也加了 b.textContent 反向断言;避免 ::before 箭头和 ☰ 叠在一起。
  • 测试用 assertRe + [^}]* 容忍 CSS 属性重排,未来增删属性不会再假阳。
  • CHANGELOG 把"为什么"写得很清楚。

建议(非阻塞,可选)

  1. reasonix_proxy.go 测试只覆盖 </head> 注入路径
    injectReasonixPrefix 实际有 3 个回退分支(</head><head> 无闭合 → <html><head>),但 TestInjectReasonixLayoutDefaults 只喂了一个完整 <html><head>...</head></body></html>,只命中第一分支。建议加 2 个测试覆盖 <head> 无闭合、<html><head> 两种 HTML 形态,验证 layoutStyle < 闭合点(或没有闭合点时的 </body> / </html> 兜底)的不变量同样成立。

  2. reasonix_proxy.go 768/769px 边界
    @media(max-width:768px) 在 768px 时匹配 → 隐藏 #mw-sidebar-toggle@media(min-width:769px) 在 769px 起匹配 → 显示 #mw-sidebar-toggle#menu-btn{display:none!important}。在 768px 这个宽度,toggle 隐藏但 native #menu-btn 没被强制隐藏(只在大屏 min-width:769px 才 !important hide)。这是上游行为、不是本 PR 引入的回归,但 PR 涉及这条 @media 链,顺手可以加一条说明或测试。

  3. reasonix_proxy.go CSS 重复声明
    font-size:15px;line-height:1#mw-sidebar-toggle#mw-sidebar-toggle::before 各写了一遍。提取到 :root 或父规则即可。不过 inline 写也方便 grep 改,单点取舍。

没发现硬伤

  • 测试 layoutStyle > headEnd 是真不变量守卫(保证 layout <style> 注入到 </head> 之前,让同特异性规则覆盖上游);这条测试加得值。
  • CSS transition 写得对:background .15s 跟默认 timing,left .25s ease 单独指定,跟原始语义一致。
  • 移动端 @media(max-width:768px) #mw-sidebar-toggle{display:none!important} 兜底正确。

关于更广范围的 8 处建议

之前我先把 PR#58 当成整条 feature/reasonix-native-ui 分支(含 #51/#57 的 25 个文件、+2233 行)来评审,产出了 8 条发现(PATCH 漏包 instanceView、preStart 与 serve env 不一致、xterm 容器遮 iframe、state.json 每请求重读、Delete 注释撒谎、lockStart mutex 泄漏、instanceView Marshal→Unmarshal、三把 mutex 可合一)。这些是真的,但不在 PR#58 的 diff 里——它们在已合入 develop 的 PRs #51/#57 里。建议另开 review 跟踪,或者下一轮 native-ui polish 时一并处理。本评论只针对 PR#58 实际范围。

b.title='Toggle sidebar';
b.addEventListener('click',function(){root.classList.toggle('mw-rx');});
document.body.appendChild(b);
}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

测试只覆盖 </head> 注入路径injectReasonixPrefix 实际有 3 个回退分支(</head><head> 无闭合 → <html><head>),但 TestInjectReasonixLayoutDefaults 只喂了完整 <html><head>...</head></body></html>,只命中第一分支。建议补 2 个测试覆盖 <head> 无闭合、<html><head> 两种 HTML 形态,验证"layout <style> 必须在所有上游 <style>/</head> 之前"这条不变量在所有分支都成立——目前真不变量守卫 layoutStyle > headEnd(test 第 405 行附近)只对 </head> 分支有意义。

Comment thread internal/app/reasonix_proxy.go Outdated
.app{grid-template-columns:var(--mw-sidebar-w,220px) 1fr}
#mw-sidebar-toggle{position:fixed;top:8px;left:8px;z-index:97;width:34px;height:34px;display:flex;align-items:center;justify-content:center;border-radius:var(--radius,8px);background:var(--panel,#222);border:1px solid var(--border,#333);color:var(--fg-2,#aaa);cursor:pointer;font-size:16px;line-height:1;transition:background .15s,left .25s ease}
#mw-sidebar-toggle{position:fixed;top:8px;left:2px;z-index:97;width:24px;height:64px;box-sizing:border-box;display:flex;align-items:center;justify-content:center;border-radius:12px;background:var(--panel,#222);border:1px solid var(--border,#333);color:var(--fg-2,#aaa);cursor:pointer;font-size:15px;line-height:1;transition:background .15s,left .25s ease}
#mw-sidebar-toggle::before{content:'▶';font-size:15px;line-height:1}

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSS 重复声明font-size:15px;line-height:1 在本行 #mw-sidebar-toggle 和下一行 #mw-sidebar-toggle::before 各写了一遍。两边得手工同步——如果以后调字号改了一边忘了另一边,按钮和箭头大小就会错位。低优先级,inline 写也方便 grep;不过提取到 :root{--mw-btn-font-size:15px} 或父规则的 CSS 变量更稳。

@linletian

Copy link
Copy Markdown
Owner Author

已逐条核对并处理评审意见(提交 beb88a8、d7adf8d,CI 全绿):

  1. 测试只覆盖 </head> 注入路径 ✅ 已修复:新增 TestInjectReasonixPrefixFallbackSites,table-driven 覆盖 <head> 无闭合、<html><head>、裸 fragment(无 html/head)三种形态,均断言注入内容存在且 layout <style> 位于 body 内容之前——与 </head> 路径同一不变量。
  2. 768/769px 边界 ✅ 已补充设计注释:明确 min-width:769px(#menu-btn 隐藏)与 max-width:768px(toggle 隐藏)不相交,768px 处 native #menu-btn 仍是可见入口,与上游断点一致、非本 PR 回归。
  3. CSS 重复声明 ✅ 已按"父规则"方案处理:#mw-sidebar-toggle::before 删除重复的 font-size:15px;line-height:1,glyph 从父按钮继承(比 :root 变量更简洁),设计注释同步说明。

8 条更广范围建议:确认属实但均不在 PR#58 的 diff 内(位于已合入 develop 的 #51/#57),按评审建议留待另开 review 跟踪,本次未纳入。

@linletian linletian left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR#58 复评(针对 beb88a8 + d7adf8d

上一轮 3 条建议全部落地,无回归。

1. ✅ beb88a8 — fallback 注入路径已覆盖

新增 TestInjectReasonixPrefixFallbackSites 覆盖 3 种 fallback HTML 形态:

  • <head> 无闭合 → 落到 <head> 分支,layout style 在 <body> 之前
  • <head> → 落到 <html> 分支,layout style 在 <body> 之前
  • <html>/<head> 的 bare fragment → layout style 在所有内容之前

不变量守卫用 layoutStyle < bodyStart(无 <body> 时退到 len(got)),跟 TestInjectReasonixLayoutDefaults 用的 layoutStyle < headEnd 守的同一条不变量(layout 必须落在任何上游 <style> 之前以保持特异性优势),合理。

测试也已跑通:go test ./internal/app/... -run TestInjectReasonix -count=1 全绿。

2. ✅ d7adf8d — 768px 边界已加文档说明

注释块新加 "768px edge" 段,明确说两条 @media 是不相交的(min-width:769pxmax-width:768px),并标注"at exactly 768px the native mobile #menu-btn remains the visible entry point (upstream's own breakpoint). Matches upstream; not a regression from this PR." 看护条件写清楚了,未来维护者不会以为是 bug。

3. ✅ d7adf8d — CSS 重复声明已去重

#mw-sidebar-toggle::before 里删掉了 font-size:15px;line-height:1,只留 content:'▶'。CSS 里 font-sizeline-height 都是 inheritable,::before 伪元素从生成它的 #mw-sidebar-toggle 继承这两个属性 → 箭头仍是 15px / 行高 1。注释里也加了一句"declared once on #mw-sidebar-toggle and inherited by the ::before pseudo-element",把意图写明。

没新发现的问题

  • 改完跑 go test ./internal/app/... -run TestInjectReasonix -count=1 全过。
  • 没引入新代码路径,diff 极小(11 + 41 行),可安全合入。

更广范围的 8 处建议

上一轮总评里列的 8 条(PATCH 漏 instanceView、preStart env 不一致、xterm 容器遮 iframe、state.json 每请求重读、Delete 注释撒谎、lockStart mutex 泄漏、instanceView Marshal→Unmarshal、三把 mutex 可合一)依然成立、依然落在已合入 develop 的 PRs #51/#57 里。本 PR 范围已无可改之处。

@linletian
linletian merged commit 6aa713d into develop Aug 12, 2026
4 checks passed
linletian added a commit that referenced this pull request Aug 12, 2026
…y, lock cleanup)

- PATCH /api/instances and POST /api/instances/restart responses now render
  through instanceView, so reasonix instances keep their web_url like GET/POST
- instanceView builds the response map directly, dropping the per-call
  Marshal->Unmarshal round-trip
- preStart env now strips inherited REASONIX_HOME/REASONIX_STATE_HOME exactly
  like serve (shared reasonix.RemoveEnv), keeping both phases on the same
  ~/.reasonix home; host-exported overrides still apply via tag env
- Manager.Stop tears down the per-instance serve-management dir and Start
  lock (Cleanup) after stopping, since the id is never reused; the driver's
  starts map no longer grows across start/stop cycles
- delete confirmation copy corrected to "Delete instance?"
- tests: PATCH web_url, Cleanup drops Start lock, preStart env strip,
  Stop cleans management dir; Restart cleanup test adapted (Stop now
  pre-cleans, seeds a stale dir instead)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant