feat(reasonix): native-ui polish — session sharing, shutdown, UI fixes + CI/test (issues #43-#49, #56) - #58
Conversation
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
left a comment
There was a problem hiding this comment.
PR#58 评审(中文白话)
范围说明:本 PR 的 base 是 c592473(PR #57 的 merge commit),gh pr diff 实际只包含 2 个提交、3 个文件:CHANGELOG.md、internal/app/reasonix_proxy.go、internal/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),并明确写了"上游.transcriptpadding 改了要重验"的看护条件。 - 箭头方向 ▶/◀ 表示"侧栏移动方向"(点 ▶ 展开到右边、点 ◀ 收回左边),比 ☰ 更直观,且零文本零 i18n。
b.textContent删了,测试也加了b.textContent反向断言;避免 ::before 箭头和 ☰ 叠在一起。- 测试用
assertRe+[^}]*容忍 CSS 属性重排,未来增删属性不会再假阳。 - CHANGELOG 把"为什么"写得很清楚。
建议(非阻塞,可选)
-
reasonix_proxy.go测试只覆盖</head>注入路径:
injectReasonixPrefix实际有 3 个回退分支(</head>→<head>无闭合 →<html>无<head>),但TestInjectReasonixLayoutDefaults只喂了一个完整<html><head>...</head></body></html>,只命中第一分支。建议加 2 个测试覆盖<head>无闭合、<html>无<head>两种 HTML 形态,验证layoutStyle < 闭合点(或没有闭合点时的</body>/</html>兜底)的不变量同样成立。 -
reasonix_proxy.go768/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 才!importanthide)。这是上游行为、不是本 PR 引入的回归,但 PR 涉及这条 @media 链,顺手可以加一条说明或测试。 -
reasonix_proxy.goCSS 重复声明:
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); | ||
| } |
There was a problem hiding this comment.
测试只覆盖 </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> 分支有意义。
| .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} |
There was a problem hiding this comment.
CSS 重复声明:font-size:15px;line-height:1 在本行 #mw-sidebar-toggle 和下一行 #mw-sidebar-toggle::before 各写了一遍。两边得手工同步——如果以后调字号改了一边忘了另一边,按钮和箭头大小就会错位。低优先级,inline 写也方便 grep;不过提取到 :root{--mw-btn-font-size:15px} 或父规则的 CSS 变量更稳。
|
已逐条核对并处理评审意见(提交 beb88a8、d7adf8d,CI 全绿):
8 条更广范围建议:确认属实但均不在 PR#58 的 diff 内(位于已合入 develop 的 #51/#57),按评审建议留待另开 review 跟踪,本次未纳入。 |
linletian
left a comment
There was a problem hiding this comment.
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:769px 与 max-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-size 和 line-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 范围已无可改之处。
…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)
概述
从
feature/reasonix-native-ui到develop的 PR,包含 native-ui 的完善与修复。主要变更
/rx/说明
当前分支相对
develop落后 1 个提交(仅 PR #50 的 merge commit,内容已包含在本分支历史中),如需可通过 GitHub "Update branch" 或gh pr update-branch同步。