feat(logs): add session reuse origin decision chain (#736)#810
Conversation
Add originDecisionTitle/Desc/Loading/Unavailable/Expand keys to logs.details.logicTrace in all 5 language dashboard.json files. Add originHint key to summary in all 5 language provider-chain.json files.
📝 WalkthroughWalkthrough该 PR 在多语言消息文件中新增“原始决策”相关本地化键，新增后端动作 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @ding113, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the observability of session reuse logic within the application's logging and tracing features. Previously, when a session reused a provider, the decision chain lacked detail. This change introduces the capability to retrieve and display the original provider selection decision from the first request in that session, providing crucial context for debugging and understanding provider allocation. This is achieved through new backend data retrieval, a secure server action, and updated UI components that dynamically load and present this information. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
github-actions
bot
added
size/XL
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively adds the functionality to display the original provider selection decision for session reuse requests. The implementation is well-structured, with a clear separation between the new repository method, server action, and UI components. The on-demand fetching of the origin chain in the UI is a good performance consideration. The addition of comprehensive unit and integration tests is also commendable. I have one suggestion regarding the handling of error messages in the new server action to improve internationalization and maintainability.
| export async function getSessionOriginChain( | ||
| sessionId: string | ||
| ): Promise<ActionResult<ProviderChainItem[] | null>> { | ||
| try { | ||
| const session = await getSession(); | ||
| if (!session) { | ||
| return { ok: false, error: "未登录" }; | ||
| } | ||
|
|
||
| if (session.user.role !== "admin") { | ||
| const userKeys = await findKeyList(session.user.id); | ||
| const userKeyValues = userKeys.map((key) => key.key); | ||
|
|
||
| const ownershipCondition = | ||
| userKeyValues.length > 0 | ||
| ? or( | ||
| eq(messageRequest.userId, session.user.id), | ||
| inArray(messageRequest.key, userKeyValues) | ||
| ) | ||
| : eq(messageRequest.userId, session.user.id); | ||
|
|
||
| const [ownedSession] = await db | ||
| .select({ id: messageRequest.id }) | ||
| .from(messageRequest) | ||
| .where( | ||
| and( | ||
| eq(messageRequest.sessionId, sessionId), | ||
| isNull(messageRequest.deletedAt), | ||
| ownershipCondition | ||
| ) | ||
| ) | ||
| .limit(1); | ||
|
|
||
| if (!ownedSession) { | ||
| return { ok: false, error: "无权访问该 Session" }; | ||
| } | ||
| } | ||
|
|
||
| const chain = await findSessionOriginChain(sessionId); | ||
| return { ok: true, data: chain ?? null }; | ||
| } catch (error) { | ||
| logger.error("获取会话来源链失败:", error); | ||
| return { ok: false, error: "获取会话来源链失败" }; | ||
| } | ||
| } |
There was a problem hiding this comment.
The error messages returned by this server action are hardcoded in Chinese (e.g., "未登录", "无权访问该 Session"). This is not ideal for internationalization.
While the current client-side implementation may not display these strings directly, it's a best practice to return error keys (e.g., errors.unauthenticated, errors.unauthorized) from server actions. The client can then use these keys to look up the appropriate translated message. This makes the action more robust and easier to maintain across different languages.
Consider refactoring to return error keys instead of hardcoded strings.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
src/repository/message.ts (1)
280-281: 缺少 JSDoc 注释文件中其他所有函数均有中文 JSDoc 注释（如
/** 创建消息请求记录 */），findSessionOriginChain缺少对应注释。建议补充
+/** + * 根据 sessionId 查询该 session 首条非 warmup 请求的 providerChain + * 用于展示会话来源链（原始选择决策） + */ export async function findSessionOriginChain(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/message.ts` around lines 280 - 281, 函数 findSessionOriginChain 缺少与项目内其他函数一致的中文 JSDoc 注释；请在 findSessionOriginChain 声明上方添加一段中文 JSDoc，简要描述该函数用途（例如“查找会话的来源链”）、列出参数 `@param` sessionId: string 的含义、标注返回值 `@returns`（例如 Promise<...> 的含义）并注明该函数是异步的，这样风格与文件中其它注释（如 “/** 创建消息请求记录 */”）保持一致。tests/unit/actions/session-origin-chain-integration.test.ts (1)
76-83: 冗余断言，可选清理第 76 行的
toEqual已完整验证ok: true和data: firstRequestChain，随后的 78-80 行 TypeScript 类型收窄分支及第 82 行的data[0]?.reason检查均已被第 76 行覆盖，属于重复断言。不影响正确性，可视情况精简。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/actions/session-origin-chain-integration.test.ts` around lines 76 - 83, The test contains redundant assertions: the initial expect(result).toEqual({ ok: true, data: firstRequestChain }) already fully asserts ok and data, so remove the duplicate checks to slim the test—specifically delete expect(result.ok).toBe(true), the manual type-narrowing if (!result.ok || !result.data) { ... }, and the redundant expect(result.data[0]?.reason).toBe("initial_selection"); keep the primary equality assertion (result equals the expected shape) and the final expect(result.data).not.toEqual(secondRequestChain) to preserve the negative-case verification.src/app/[locale]/dashboard/logs/_components/error-details-dialog.test.tsx (2)
1160-1164: 建议在click前断言 trigger 存在如果
trigger未找到，click(null)会静默返回，后续断言可能因为根本没触发展开而误通过（或给出误导性的失败信息）。建议加一行显式断言。建议
const trigger = Array.from(container.querySelectorAll("button")).find((button) => button.textContent?.includes("View original selection") ); - click(trigger ?? null); + expect(trigger).toBeTruthy(); + click(trigger!);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/logs/_components/error-details-dialog.test.tsx around lines 1160 - 1164, 在测试中对通过 container.querySelectorAll(...).find(...) 得到的 trigger 进行显式存在断言再调用 click；也就是说在调用 click(trigger) 之前对变量 trigger 使用断言（例如 expect(trigger).toBeDefined()/toBeTruthy()）以确保 find 找到 "View original selection" 的按钮，避免在 trigger 为 null 时静默调用 click 导致后续断言误导性通过或难以定位失败点。确保断言出现在同一测试块中，紧接在定义 trigger（const trigger = ...）之后再执行 click(trigger).
1093-1174: 缺少成功获取 originChain 数据时的渲染测试当前三个测试覆盖了：trigger 可见性、默认折叠、展开后 data=null 的降级文案。但缺少
getSessionOriginChain返回有效数据时的渲染验证（例如显示originDecisionTitle、候选提供商列表、健康检查状态等）。建议补充一个测试用例，模拟getSessionOriginChainMock.mockResolvedValue({ ok: true, data: {...} })的成功场景，验证原始选择决策链信息的完整渲染。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/logs/_components/error-details-dialog.test.tsx around lines 1093 - 1174, Add a new test for ErrorDetailsDialog that simulates a successful origin chain fetch by calling getSessionOriginChainMock.mockResolvedValue({ ok: true, data: { /* minimal originChain with originDecisionTitle, providers, health statuses */ } }), render the component with a sessionId (e.g., "sess-origin-success") and providerChain containing a session_reuse entry, trigger the "View original selection" button, await async updates, then assert getSessionOriginChainMock was called once with that sessionId and that the DOM contains the originDecisionTitle text, the expected provider names (candidate list), and any health/check status strings you expect the component to render.src/app/[locale]/dashboard/logs/_components/error-details-dialog/components/LogicTraceTab.tsx (2)
385-396: 概率格式化不一致：此处使用Math.round而非formatProbability同一文件中 line 508 使用了
formatProbability(c.probability)工具函数来格式化概率值，但这里直接用Math.round(c.probability * 100)手工计算。formatProbability还处理了 NaN、越界等边界情况（参见测试中对 probability=100 和 NaN 的覆盖）。建议保持一致以避免边界行为差异。建议：统一使用 formatProbability
- {i > 0 && ", "} - {c.name} - {c.probability !== undefined && ( - <span className="text-muted-foreground"> - {" "} - ({Math.round(c.probability * 100)}%) - </span> - )} + {i > 0 && ", "} + {c.name} + {(() => { + const formatted = formatProbability(c.probability); + return formatted ? ( + <span className="text-muted-foreground"> + {" "}({formatted}) + </span> + ) : null; + })()}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/logs/_components/error-details-dialog/components/LogicTraceTab.tsx around lines 385 - 396, The probability rendering in LogicTraceTab.tsx's ctx.candidatesAtPriority.map uses Math.round(c.probability * 100) which is inconsistent with the rest of the file; replace that manual calculation with the shared formatter by calling formatProbability(c.probability) so you reuse the same NaN/overflow handling and test-covered behavior; ensure you import/keep the existing formatProbability function in the module and pass c.probability (or guard undefined the same way other usages do) when rendering the trailing "(...%)" span.
303-406: 异步获取缺少组件卸载保护如果用户展开 Collapsible 触发
getSessionOriginChain请求后立即关闭对话框（组件卸载），.then/.catch中的setState调用会作用于已卸载的组件。虽然 React 18+ 不再为此抛出警告，但更好的做法是用一个cancelled标志来避免不必要的状态更新。此外，
onOpenChange回调中直接调用 server action 并处理 Promise——如果后续需要增加取消逻辑或去抖，当前结构不便扩展。建议：使用 cancelled 标志保护异步回调
可以考虑将 fetch 逻辑提取到
useEffect中，由originOpen驱动，利用 cleanup 函数实现取消：+ // Replace onOpenChange fetch with useEffect + React.useEffect(() => { + if (!originOpen || originChain !== undefined || originLoading) return; + let cancelled = false; + setOriginLoading(true); + getSessionOriginChain(sessionId!) + .then((result) => { + if (!cancelled) setOriginChain(result.ok ? result.data : null); + }) + .catch(() => { + if (!cancelled) setOriginChain(null); + }) + .finally(() => { + if (!cancelled) setOriginLoading(false); + }); + return () => { cancelled = true; }; + }, [originOpen, originChain, originLoading, sessionId]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/logs/_components/error-details-dialog/components/LogicTraceTab.tsx around lines 303 - 406, The async fetch started in the Collapsible onOpenChange (calling getSessionOriginChain) can update state after the component unmounts; move the fetch logic out of the inline onOpenChange and into a useEffect driven by originOpen/sessionId, and protect state updates with a cancelled flag (or AbortController) in the effect cleanup so setOriginChain/setOriginLoading are skipped when cancelled/unmounted; update the Collapsible onOpenChange to only toggle originOpen and let the new useEffect perform getSessionOriginChain and handle loading/result/cancelation for originChain/originLoading.src/actions/session-origin-chain.ts (1)
13-15: 建议对sessionId做基本的非空校验
sessionId参数没有做空字符串或格式校验。虽然空字符串只会导致查询无结果（行为安全），但提前返回可以避免不必要的数据库查询和权限检查。建议：增加输入校验
export async function getSessionOriginChain( sessionId: string ): Promise<ActionResult<ProviderChainItem[] | null>> { try { + if (!sessionId) { + return { ok: false, error: "无效的 Session ID" }; + } const session = await getSession();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/session-origin-chain.ts` around lines 13 - 15, Add a basic non-empty validation at the start of getSessionOriginChain for the sessionId parameter: if sessionId is null/undefined or an empty/whitespace string, immediately return an ActionResult indicating a validation failure (e.g., success:false or an error payload with message like "sessionId is required") instead of proceeding to database queries or permission checks; update the function (getSessionOriginChain) to perform this check before any DB/permission logic that produces ProviderChainItem[] | null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/app/`[locale]/dashboard/logs/_components/error-details-dialog/components/LogicTraceTab.tsx:
- Around line 357-363: In LogicTraceTab.tsx the i18n key
t("logicTrace.healthyCount", { count: ctx.enabledProviders }) is semantically
wrong: healthyCount implies post-health-check results while ctx.enabledProviders
represents enabled providers; change the call to use the correct key or
value—either use t("logicTrace.enabledCount", { count: ctx.enabledProviders })
(or an existing providersCount key) to reflect enabled providers, or if you
intended to show healthy providers use ctx.afterHealthCheck as the count with
t("logicTrace.healthyCount"). Ensure the displayed translation key matches the
variable (ctx.enabledProviders vs ctx.afterHealthCheck) and update the i18n
resource if needed.
In `@src/app/`[locale]/dashboard/logs/_components/provider-chain-popover.tsx:
- Around line 261-265: sessionReuseItem.selectionMethod is a raw snake_case enum
being passed into tChain("summary.originHint") and shown to users; add a new
i18n namespace selectionMethods in provider-chain.json with keys like
selectionMethods.weighted_random, selectionMethods.group_filtered,
selectionMethods.fail_open_fallback etc., and change the render to map the enum
to its translated label by passing
tChain(`selectionMethods.${sessionReuseItem.selectionMethod}`) as the method
param to tChain("summary.originHint") (keep using
sessionReuseItem.selectionMethod to derive the key and fall back to the raw
value or a generic label if the translation is missing).
In `@src/repository/message.ts`:
- Around line 280-301: The exported repository function findSessionOriginChain
is in a "use server" file and therefore becomes a callable Server Action
(bypassing auth); move this function out of the "use server" module into a
normal server-side module (remove the "use server" directive from the module
that defines findSessionOriginChain) so it is no longer exposed as a Server
Action, keep the implementation and signature identical, and update any callers
(e.g., getSessionOriginChain in session-origin-chain action) to import the
repository function from the new module; ensure only action modules (the
dedicated "use server" files) export Server Actions and perform authentication
before calling repository functions.
In `@tests/unit/actions/session-origin-chain.test.ts`:
- Around line 31-108: Add two tests for getSessionOriginChain: (1) "non-admin
without access" — mock getSessionMock to return a non-admin user, mock
findKeyListMock to return keys and set dbLimitMock.mockResolvedValue([]) to
simulate ownedSession empty, call getSessionOriginChain("sess-no-access") and
assert it returns { ok: false, error: "无权访问该 Session" } and that
findSessionOriginChainMock was not called; (2) "error thrown" — mock
getSessionMock to return an admin user and make
findSessionOriginChainMock.mockRejectedValue(new Error("DB error")), call
getSessionOriginChain("sess-error") and assert it returns { ok: false, error:
"获取会话来源链失败" } to cover the exception path. Ensure you import
getSessionOriginChain from "@/actions/session-origin-chain" like other tests and
reset/prepare mocks (getSessionMock, findSessionOriginChainMock,
findKeyListMock, dbLimitMock) in beforeEach.
In `@tests/unit/repository/message-origin-chain.test.ts`:
- Around line 179-195: The test is incorrectly duplicating the "no results"
path; to exercise the code path that returns null when a row exists but its
providerChain is null, change the select mock used in the "null providerChain"
test (the vi.fn/select mock that returns createThenableQuery([])) to return
createThenableQuery([{ providerChain: null }]) so findSessionOriginChain
receives a row with providerChain null and triggers the if (!row?.providerChain)
return null branch; keep other mocks (db.execute) the same and then assert
result is null.
---
Nitpick comments:
In `@src/actions/session-origin-chain.ts`:
- Around line 13-15: Add a basic non-empty validation at the start of
getSessionOriginChain for the sessionId parameter: if sessionId is
null/undefined or an empty/whitespace string, immediately return an ActionResult
indicating a validation failure (e.g., success:false or an error payload with
message like "sessionId is required") instead of proceeding to database queries
or permission checks; update the function (getSessionOriginChain) to perform
this check before any DB/permission logic that produces ProviderChainItem[] |
null.
In `@src/app/`[locale]/dashboard/logs/_components/error-details-dialog.test.tsx:
- Around line 1160-1164: 在测试中对通过 container.querySelectorAll(...).find(...) 得到的
trigger 进行显式存在断言再调用 click；也就是说在调用 click(trigger) 之前对变量 trigger 使用断言（例如
expect(trigger).toBeDefined()/toBeTruthy()）以确保 find 找到 "View original selection"
的按钮，避免在 trigger 为 null 时静默调用 click 导致后续断言误导性通过或难以定位失败点。确保断言出现在同一测试块中，紧接在定义
trigger（const trigger = ...）之后再执行 click(trigger).
- Around line 1093-1174: Add a new test for ErrorDetailsDialog that simulates a
successful origin chain fetch by calling
getSessionOriginChainMock.mockResolvedValue({ ok: true, data: { /* minimal
originChain with originDecisionTitle, providers, health statuses */ } }), render
the component with a sessionId (e.g., "sess-origin-success") and providerChain
containing a session_reuse entry, trigger the "View original selection" button,
await async updates, then assert getSessionOriginChainMock was called once with
that sessionId and that the DOM contains the originDecisionTitle text, the
expected provider names (candidate list), and any health/check status strings
you expect the component to render.
In
`@src/app/`[locale]/dashboard/logs/_components/error-details-dialog/components/LogicTraceTab.tsx:
- Around line 385-396: The probability rendering in LogicTraceTab.tsx's
ctx.candidatesAtPriority.map uses Math.round(c.probability * 100) which is
inconsistent with the rest of the file; replace that manual calculation with the
shared formatter by calling formatProbability(c.probability) so you reuse the
same NaN/overflow handling and test-covered behavior; ensure you import/keep the
existing formatProbability function in the module and pass c.probability (or
guard undefined the same way other usages do) when rendering the trailing
"(...%)" span.
- Around line 303-406: The async fetch started in the Collapsible onOpenChange
(calling getSessionOriginChain) can update state after the component unmounts;
move the fetch logic out of the inline onOpenChange and into a useEffect driven
by originOpen/sessionId, and protect state updates with a cancelled flag (or
AbortController) in the effect cleanup so setOriginChain/setOriginLoading are
skipped when cancelled/unmounted; update the Collapsible onOpenChange to only
toggle originOpen and let the new useEffect perform getSessionOriginChain and
handle loading/result/cancelation for originChain/originLoading.
In `@src/repository/message.ts`:
- Around line 280-281: 函数 findSessionOriginChain 缺少与项目内其他函数一致的中文 JSDoc 注释；请在
findSessionOriginChain 声明上方添加一段中文 JSDoc，简要描述该函数用途（例如“查找会话的来源链”）、列出参数 `@param`
sessionId: string 的含义、标注返回值 `@returns`（例如 Promise<...>
的含义）并注明该函数是异步的，这样风格与文件中其它注释（如 “/** 创建消息请求记录 */”）保持一致。
In `@tests/unit/actions/session-origin-chain-integration.test.ts`:
- Around line 76-83: The test contains redundant assertions: the initial
expect(result).toEqual({ ok: true, data: firstRequestChain }) already fully
asserts ok and data, so remove the duplicate checks to slim the
test—specifically delete expect(result.ok).toBe(true), the manual type-narrowing
if (!result.ok || !result.data) { ... }, and the redundant
expect(result.data[0]?.reason).toBe("initial_selection"); keep the primary
equality assertion (result equals the expected shape) and the final
expect(result.data).not.toEqual(secondRequestChain) to preserve the
negative-case verification.
src/app/[locale]/dashboard/logs/_components/error-details-dialog/components/LogicTraceTab.tsx
Show resolved
Hide resolved
| export async function findSessionOriginChain( | ||
| sessionId: string | ||
| ): Promise<ProviderChainItem[] | null> { | ||
| const [row] = await db | ||
| .select({ | ||
| providerChain: messageRequest.providerChain, | ||
| }) | ||
| .from(messageRequest) | ||
| .where( | ||
| and( | ||
| eq(messageRequest.sessionId, sessionId), | ||
| isNull(messageRequest.deletedAt), | ||
| EXCLUDE_WARMUP_CONDITION, | ||
| sql`${messageRequest.providerChain} IS NOT NULL` | ||
| ) | ||
| ) | ||
| .orderBy(asc(messageRequest.requestSequence)) | ||
| .limit(1); | ||
|
|
||
| if (!row?.providerChain) return null; | ||
| return row.providerChain as ProviderChainItem[]; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if "use server" exists at top of src/repository/message.ts
head -5 src/repository/message.tsRepository: ding113/claude-code-hub
Length of output: 284
🏁 Script executed:
#!/bin/bash
# Find if src/actions/session-origin-chain.ts exists and check its auth
if [ -f src/actions/session-origin-chain.ts ]; then
echo "=== src/actions/session-origin-chain.ts found ==="
head -100 src/actions/session-origin-chain.ts
else
echo "File not found, searching for similar names..."
find . -name "*session*" -type f 2>/dev/null | grep -E '\.(ts|tsx)$' | head -20
fiRepository: ding113/claude-code-hub
Length of output: 1864
🏁 Script executed:
#!/bin/bash
# Check all repository functions in message.ts to confirm pre-existing pattern
rg -n '"use server"' src/repository/ -A 20 | head -50Repository: ding113/claude-code-hub
Length of output: 3050
🏁 Script executed:
#!/bin/bash
# Search for usages of findSessionOriginChain to see if it's accessed without proper auth
rg -n "findSessionOriginChain" --type ts --type tsx -B 2 -A 2Repository: ding113/claude-code-hub
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Search for usages of findSessionOriginChain
rg -n "findSessionOriginChain" --type tsRepository: ding113/claude-code-hub
Length of output: 2768
🏁 Script executed:
#!/bin/bash
# Also check if there are any client-side calls that might bypass the auth layer
rg -n "getSessionOriginChain" --type ts --type tsx -B 1 -A 1Repository: ding113/claude-code-hub
Length of output: 94
findSessionOriginChain 在 "use server" 文件中被导出，可被直接调用为 Server Action，绕过鉴权
src/repository/message.ts 带有 "use server" 指令，使得 findSessionOriginChain 成为可直接调用的 Server Action 端点。虽然当前代码仅通过 src/actions/session-origin-chain.ts 中的 getSessionOriginChain 访问它（该函数包含身份验证和会话所有权检查），但 findSessionOriginChain 本身没有身份验证，可以被知晓其 Server Action ID 的攻击者直接调用，绕过 getSessionOriginChain 中的鉴权逻辑。
这是该文件和其他 repository 文件（webhook-targets.ts、user.ts、sensitive-words.ts 等）中的既有架构模式。建议将 repository 层函数从 "use server" 文件迁移到普通服务端模块，仅在专用 actions 文件中保留该指令，以确保所有数据访问都经过完整的身份验证流程。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/repository/message.ts` around lines 280 - 301, The exported repository
function findSessionOriginChain is in a "use server" file and therefore becomes
a callable Server Action (bypassing auth); move this function out of the "use
server" module into a normal server-side module (remove the "use server"
directive from the module that defines findSessionOriginChain) so it is no
longer exposed as a Server Action, keep the implementation and signature
identical, and update any callers (e.g., getSessionOriginChain in
session-origin-chain action) to import the repository function from the new
module; ensure only action modules (the dedicated "use server" files) export
Server Actions and perform authentication before calling repository functions.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR implements on-demand fetching of the original provider selection decision chain for session reuse requests. The implementation is well-structured with proper separation of concerns (repository, action, UI) and includes comprehensive test coverage (52 new tests). The code follows existing patterns in the codebase and includes proper i18n support for all 5 languages.
PR Size: XL
- Lines changed: 1110 (922 additions, 188 deletions)
- Files changed: 19
Split Suggestion for Future: This PR could potentially be split into 2 parts: (1) backend changes (repository + action + tests) and (2) UI changes (components + i18n). This would make reviews more focused and allow parallel work.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
No issues requiring changes were identified in this PR.
Review Coverage
- Logic and correctness - Clean implementation with proper flow detection for session reuse
- Security (OWASP Top 10) - Proper auth gating with admin/user permission scoping
- Error handling - Graceful degradation implemented, errors logged appropriately
- Type safety - Proper TypeScript types used,
ProviderChainItemimported correctly - Documentation accuracy - i18n keys added consistently across all 5 languages
- Test coverage - Adequate (52 new tests covering repository, action, UI, and integration layers)
- Code clarity - Good, follows existing patterns in the codebase
Notable Implementation Details
-
On-demand Loading: The origin chain is fetched only when the user expands the collapsible section (line 845 in LogicTraceTab.tsx), not on dialog open - this optimizes performance.
-
Permission Scoping: The action properly checks ownership for non-admin users (session-origin-chain.ts lines 588-615), ensuring users can only access their own session data.
-
Warmup Exclusion: Repository query correctly uses
EXCLUDE_WARMUP_CONDITIONto filter out warmup requests (message.ts lines 1064), ensuring the first real request is returned. -
Silent Degradation: UI handles unavailable origin chains gracefully by showing "Original decision record unavailable" message (LogicTraceTab.tsx lines 869-873).
Automated review by Claude AI
| .limit(1); | ||
|
|
||
| if (!ownedSession) { | ||
| return { ok: false, error: "无权访问该 Session" }; |
There was a problem hiding this comment.
[HIGH] [TEST-MISSING-CRITICAL] Permission-denied branch untested
Why this is a problem: This branch enforces non-admin access control. Without a unit test, it’s easy to regress authorization behavior without noticing.
Evidence: src/actions/session-origin-chain.ts:46-48
if (!ownedSession) {
return { ok: false, error: "无权访问该 Session" };
}Suggested fix:
test("non-admin: returns permission denied when session not owned", async () => {
getSessionMock.mockResolvedValue({ user: { id: 2, role: "user" } });
findKeyListMock.mockResolvedValue([{ key: "user-key-1" }]);
dbLimitMock.mockResolvedValueOnce([]);
const { getSessionOriginChain } = await import("@/actions/session-origin-chain");
const result = await getSessionOriginChain("sess-forbidden");
expect(result).toEqual({ ok: false, error: "无权访问该 Session" });
expect(findSessionOriginChainMock).not.toHaveBeenCalled();
});
| try { | ||
| const session = await getSession(); | ||
| if (!session) { | ||
| return { ok: false, error: "未登录" }; |
There was a problem hiding this comment.
[MEDIUM] [STANDARD-VIOLATION] Action errors lack errorCode for i18n
Why this is a problem: This action returns hardcoded error text, which can leak to user-facing UI/API responses and cannot be translated reliably.
Evidence:
src/actions/session-origin-chain.ts:19return { ok: false, error: "未登录" };src/actions/session-origin-chain.ts:47return { ok: false, error: "无权访问该 Session" };src/actions/session-origin-chain.ts:55return { ok: false, error: "获取会话来源链失败" };
This violates CLAUDE.md:14:
3. **i18n Required** - All user-facing strings must use i18n (5 languages supported). Never hardcode display text
And it skips the recommended i18n path in src/actions/types.ts:6-10:
@property errorCode - Error code for i18n translation (optional, recommended for new code)
Suggested fix:
import { ERROR_CODES } from "@/lib/utils/error-messages";
if (!session) {
return { ok: false, error: "未登录", errorCode: ERROR_CODES.UNAUTHORIZED };
}
...
if (!ownedSession) {
return { ok: false, error: "无权访问该 Session", errorCode: ERROR_CODES.PERMISSION_DENIED };
}
...
return {
ok: false,
error: "获取会话来源链失败",
errorCode: ERROR_CODES.OPERATION_FAILED,
};- fix semantic mismatch: use providersCount key for enabledProviders display - fix probability formatting: use formatProbability() instead of Math.round - fix i18n: translate selectionMethod enum via selectionMethods namespace - add selectionMethods translations to all 5 language files - add JSDoc to findSessionOriginChain repository function - fix test: null providerChain mock now returns row with null providerChain - fix test: add assertion before trigger click in error-details-dialog test - add 2 missing test cases: non-admin unauthorized + exception path
There was a problem hiding this comment.
Code Review Summary
This is an XL PR spanning backend repository logic, a new server action, UI wiring, and i18n message updates. Review focus was on the new session-origin-chain fetch path (authz, error handling/i18n, and tests) and the UI integration.
PR Size: XL
- Lines changed: 1110
- Files changed: 19
- Split suggestion: Consider splitting into (1) backend (repository + action) (2) UI changes (LogicTraceTab + popover) (3) i18n message updates (4) tests, to reduce review surface area.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 1 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 1 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
None.
High Priority Issues (Should Fix)
- (Confidence 90) permission-denied branch is untested; add a unit test for the non-admin “not owned” path.
- (Confidence 85) returns hardcoded error strings without ; add for i18n compliance.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
There was a problem hiding this comment.
Code Review Summary
This is an XL PR spanning backend repository logic, a new server action, UI wiring, and i18n message updates. Review focus was on the new session-origin-chain fetch path (authz, error handling/i18n, and tests) and the UI integration.
PR Size: XL
- Lines changed: 1110
- Files changed: 19
- Split suggestion: Consider splitting into (1) backend (repository + action) (2) UI changes (LogicTraceTab + popover) (3) i18n message updates (4) tests, to reduce review surface area.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 1 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 1 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
None.
High Priority Issues (Should Fix)
- (Confidence 90)
src/actions/session-origin-chain.tspermission-denied branch is untested; add a unit test for the non-admin “not owned” path. - (Confidence 85)
src/actions/session-origin-chain.tsreturns hardcoded error strings withouterrorCode; adderrorCodefor i18n compliance.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
There was a problem hiding this comment.
PR detected: #810 “feat(logs): add session reuse origin decision chain (#736)”
- Applied size label:
size/XL(Additions: 922, deletions: 188, files changed: 19; total lines changed: 1110) - Posted 2 inline review comments (both on diff lines in
src/actions/session-origin-chain.ts):- [HIGH]
[TEST-MISSING-CRITICAL]Missing unit test for the non-admin permission-denied branch atsrc/actions/session-origin-chain.ts:46-48 - [MEDIUM]
[STANDARD-VIOLATION]Hardcoded error strings withouterrorCodeatsrc/actions/session-origin-chain.ts:19,src/actions/session-origin-chain.ts:47,src/actions/session-origin-chain.ts:55
- [HIGH]
- Submitted the required PR review summary via
gh pr review --comment(includes XL split suggestions and the issue matrix).
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
tests/unit/repository/message-origin-chain.test.ts (3)
143-177: "no data" 与 "all warmup" 两个测试在 mock 层面完全等价，测试相同的代码路径两个测试均使用
createThenableQuery([])并断言result为null。由于 warmup 过滤在 SQL 层通过EXCLUDE_WARMUP_CONDITION实现，mock 无法区分"session 不存在"和"全部记录被 warmup 过滤"两种场景——两者在应用层均表现为空数组解构后row = undefined，触发同一return null路径。若希望真正区分这两种场景，需在集成测试层使用真实数据库验证。当前如两个测试仅作文档目的保留，建议在测试注释中说明此局限性，以免后续维护者误判覆盖范围。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/repository/message-origin-chain.test.ts` around lines 143 - 177, The two tests call findSessionOriginChain and both mock db.select to return createThenableQuery([]), meaning they exercise the same code path (empty result -> row undefined -> return null) so they do not distinguish "session not found" from "all warmup filtered"; update the tests to avoid redundancy by either removing one of them or keeping both but adding a clear comment in the test(s) that explains the limitation (mocked SQL cannot simulate EXCLUDE_WARMUP_CONDITION and only an integration test with a real DB can differentiate those scenarios), and reference the involved symbols findSessionOriginChain, createThenableQuery and EXCLUDE_WARMUP_CONDITION in that comment so future maintainers understand why the mock tests are equivalent.
4-39:sqlToString依赖 Drizzle ORM 内部 AST 结构，存在脆弱性风险该工具函数通过探测
.name、.value、.queryChunks等属性推断 SQL 字符串表示，绑定了 Drizzle ORM 的私有内部数据结构。若 Drizzle 在后续版本中调整 AST 格式，walk函数将静默返回空字符串，导致 happy path 中expect(whereSql).toContain("warmup")等断言以误导性的方式失败，而非因功能回退而失败。更稳健的替代方案：在
wheremock 捕获到参数后，直接对比捕获到的参数对象引用（例如检查EXCLUDE_WARMUP_CONDITION对象是否在whereArgs中出现），而非依赖字符串化的 SQL 内容。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/repository/message-origin-chain.test.ts` around lines 4 - 39, The current sqlToString helper (and its inner walk) relies on Drizzle ORM internals and is brittle; replace uses in the test with a direct assertion on the captured where parameters instead: change the test to read the mocked where call arguments (where mock's captured calls / whereArgs) and assert that the actual EXCLUDE_WARMUP_CONDITION object/reference appears in those args (or use a small identity-search helper that traverses the captured args to find the exact object reference) rather than converting AST to string via sqlToString; update any assertions like expect(whereSql).toContain("warmup") to use expect(whereArgs).toContain(EXCLUDE_WARMUP_CONDITION) or equivalent identity search against the where mock/captured parameters.
115-141: "warmup skip" 测试名称具有误导性，且未提供超越 happy path 的额外覆盖该测试意图描述"当首条记录为 warmup 时，应跳过并返回后续非 warmup 的 chain"，但实际上：
- warmup 过滤在 SQL 层执行（
EXCLUDE_WARMUP_CONDITION在WHERE子句中），mock 完全绕过了 SQL 执行，因此无法在该层级验证 warmup 跳过行为。- 测试数据与描述矛盾：返回的 chain 使用
reason: "session_reuse"/selectionMethod: "session_reuse"，这是会话复用场景，并非"跳过 warmup 后的首条非 warmup 记录"所应呈现的数据（预期应为reason: "initial_selection"等）。- 代码路径与 happy path 完全相同：均为"mock 返回非空数组 → 函数返回该数据"，没有新增覆盖路径。happy path（第 103–104 行）已通过
whereArgs检查验证了EXCLUDE_WARMUP_CONDITION的存在。建议删除该测试或将其改写为集成测试，在真实 DB 层面验证 warmup 记录被正确过滤。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/repository/message-origin-chain.test.ts` around lines 115 - 141, The "warmup skip" unit test is misleading and ineffective because warmup filtering happens in SQL via EXCLUDE_WARMUP_CONDITION (verified in the existing whereArgs happy-path test), yet this test fully mocks the DB and never exercises that SQL behavior; additionally the mocked chain uses reason/selectionMethod "session_reuse" which contradicts the test intent. Fix by either deleting the test or converting it to an integration test that runs against the real DB to verify EXCLUDE_WARMUP_CONDITION actually filters warmup rows, or if you must keep it as a unit test, change the mock for findSessionOriginChain to return two rows (first marked warmup, second non-warmup) and assert that findSessionOriginChain returns the non-warmup chain (use a non-warmup reason like "initial_selection"/selectionMethod "initial_selection") so the test actually verifies the skipping behavior.src/app/[locale]/dashboard/logs/_components/error-details-dialog.test.tsx (2)
21-25:hasSessionMessagesMock缺少mockReset()调用，导致调用历史跨测试累积
getSessionOriginChainMock在beforeEach中先调用了mockReset()（清除调用记录和历史），而hasSessionMessagesMock只调用了mockResolvedValue()（仅覆盖默认返回值，不清除调用历史）。两者处理方式不一致。若后续测试断言hasSessionMessagesMock.mock.calls.length，将得到跨测试的累积值，导致误判。♻️ 建议统一重置两个 mock
beforeEach(() => { + hasSessionMessagesMock.mockReset(); hasSessionMessagesMock.mockResolvedValue({ ok: true, data: false }); getSessionOriginChainMock.mockReset(); getSessionOriginChainMock.mockResolvedValue({ ok: false, error: "mock" }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/logs/_components/error-details-dialog.test.tsx around lines 21 - 25, The beforeEach resets getSessionOriginChainMock but not hasSessionMessagesMock, causing call history to leak between tests; update the beforeEach so hasSessionMessagesMock is also reset (call hasSessionMessagesMock.mockReset()) before setting its default resolved value (hasSessionMessagesMock.mockResolvedValue({...})) to ensure both mocks use the same reset+setup pattern and avoid cross-test call accumulation.
1093-1175: 原始决策链功能的测试覆盖率不足，且缺少失败时的清理保障新增的 3 个测试用例目前覆盖：触发器可见性、默认折叠状态、
data: null时显示"不可用"。以下重要路径尚未覆盖，估计覆盖率约 57%，低于编码规范要求的 80%：
- 加载态：展开后 fetch 进行中时，
"Loading original decision..."是否短暂显示- 成功路径（happy path）：
getSessionOriginChain返回有效的原始决策链时，组件是否正确渲染其内容- 错误路径（
ok: false）：展开后 mock 返回{ ok: false }时的 UI 表现此外，
unmount()直接在测试末尾调用而未包裹在try/finally中——若中途某个expect抛出异常，DOM 将不被清理，可能污染后续测试。♻️ 建议补充测试用例（happy path 示例）
test("shows origin chain data after expand when origin decision is available", async () => { const mockChain = [{ id: 1, name: "p1", reason: "initial_selection" }]; getSessionOriginChainMock.mockResolvedValue({ ok: true, data: mockChain }); const { container, unmount } = renderClientWithIntl( <ErrorDetailsDialog externalOpen statusCode={200} errorMessage={null} sessionId={"sess-origin-happy"} providerChain={[{ id: 1, name: "p1", reason: "session_reuse" }] as any} /> ); try { const trigger = Array.from(container.querySelectorAll("button")).find( (button) => button.textContent?.includes("View original selection"), ); expect(trigger).toBeTruthy(); click(trigger!); await act(async () => { await new Promise<void>((resolve) => setTimeout(resolve, 0)); }); expect(getSessionOriginChainMock).toHaveBeenCalledWith("sess-origin-happy"); // 验证成功加载后的内容，如 "Original Selection Decision" 标题或决策数据 expect(container.textContent).toContain("Original Selection Decision"); } finally { unmount(); } });As per coding guidelines:
**/*.test.{ts,tsx,js,jsx}: "All new features must have unit test coverage of at least 80%".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/logs/_components/error-details-dialog.test.tsx around lines 1093 - 1175, Tests for ErrorDetailsDialog are missing coverage for loading, success, and error fetch paths and also call unmount() outside a try/finally which can leak DOM on failures; add new tests that (1) mock getSessionOriginChainMock to resolve with ok:true and data to assert the "happy path" rendering after clicking the "View original selection" trigger (use ErrorDetailsDialog and renderClientWithIntl to render, find the trigger button, click it, await the fetch, then assert expected content), (2) mock getSessionOriginChainMock to resolve with ok:false and assert the error UI shown after expanding, and (3) mock a pending promise or assert the transient "Loading original decision..." state while the fetch is in-flight; wrap each test's unmount() in try/finally (or use finally block) to ensure cleanup even if assertions fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@messages/zh-TW/provider-chain.json`:
- Line 23: Update the zh-TW message for the JSON key originHint to use the
customary Traditional Chinese wording for "via" by replacing "通過" with "透過" in
the string "會話複用 - 最初通過 {method} 選擇" so it becomes "會話複用 - 最初透過 {method} 選擇";
ensure only the wording is changed and the placeholder {method} remains
untouched.
In `@src/app/`[locale]/dashboard/logs/_components/error-details-dialog.test.tsx:
- Around line 1160-1175: Wrap the button click and the subsequent microtask
drain inside a single act block to ensure React tracks all state updates: call
click(trigger!) within an await act(async () => { ... }) and inside that act
await a full microtask/tick drain (e.g., await new Promise(resolve =>
setTimeout(resolve, 0))) instead of a single Promise.resolve(); keep the
existing assertions for getSessionOriginChainMock and container text, and ensure
trigger is truthy before clicking (trigger variable and
getSessionOriginChainMock referenced to locate the test).
---
Duplicate comments:
In `@tests/unit/repository/message-origin-chain.test.ts`:
- Around line 179-195: The test was previously mocking an empty result which
didn't hit the defensive branch in findSessionOriginChain; update the select
mock used in this test (selectMock) to return a thenable query containing an
object with providerChain: null (e.g. createThenableQuery([{ providerChain: null
}])) so that the condition in findSessionOriginChain (if (!row?.providerChain)
return null) is exercised and the test asserts null correctly.
---
Nitpick comments:
In `@src/app/`[locale]/dashboard/logs/_components/error-details-dialog.test.tsx:
- Around line 21-25: The beforeEach resets getSessionOriginChainMock but not
hasSessionMessagesMock, causing call history to leak between tests; update the
beforeEach so hasSessionMessagesMock is also reset (call
hasSessionMessagesMock.mockReset()) before setting its default resolved value
(hasSessionMessagesMock.mockResolvedValue({...})) to ensure both mocks use the
same reset+setup pattern and avoid cross-test call accumulation.
- Around line 1093-1175: Tests for ErrorDetailsDialog are missing coverage for
loading, success, and error fetch paths and also call unmount() outside a
try/finally which can leak DOM on failures; add new tests that (1) mock
getSessionOriginChainMock to resolve with ok:true and data to assert the "happy
path" rendering after clicking the "View original selection" trigger (use
ErrorDetailsDialog and renderClientWithIntl to render, find the trigger button,
click it, await the fetch, then assert expected content), (2) mock
getSessionOriginChainMock to resolve with ok:false and assert the error UI shown
after expanding, and (3) mock a pending promise or assert the transient "Loading
original decision..." state while the fetch is in-flight; wrap each test's
unmount() in try/finally (or use finally block) to ensure cleanup even if
assertions fail.
In `@tests/unit/repository/message-origin-chain.test.ts`:
- Around line 143-177: The two tests call findSessionOriginChain and both mock
db.select to return createThenableQuery([]), meaning they exercise the same code
path (empty result -> row undefined -> return null) so they do not distinguish
"session not found" from "all warmup filtered"; update the tests to avoid
redundancy by either removing one of them or keeping both but adding a clear
comment in the test(s) that explains the limitation (mocked SQL cannot simulate
EXCLUDE_WARMUP_CONDITION and only an integration test with a real DB can
differentiate those scenarios), and reference the involved symbols
findSessionOriginChain, createThenableQuery and EXCLUDE_WARMUP_CONDITION in that
comment so future maintainers understand why the mock tests are equivalent.
- Around line 4-39: The current sqlToString helper (and its inner walk) relies
on Drizzle ORM internals and is brittle; replace uses in the test with a direct
assertion on the captured where parameters instead: change the test to read the
mocked where call arguments (where mock's captured calls / whereArgs) and assert
that the actual EXCLUDE_WARMUP_CONDITION object/reference appears in those args
(or use a small identity-search helper that traverses the captured args to find
the exact object reference) rather than converting AST to string via
sqlToString; update any assertions like expect(whereSql).toContain("warmup") to
use expect(whereArgs).toContain(EXCLUDE_WARMUP_CONDITION) or equivalent identity
search against the where mock/captured parameters.
- Around line 115-141: The "warmup skip" unit test is misleading and ineffective
because warmup filtering happens in SQL via EXCLUDE_WARMUP_CONDITION (verified
in the existing whereArgs happy-path test), yet this test fully mocks the DB and
never exercises that SQL behavior; additionally the mocked chain uses
reason/selectionMethod "session_reuse" which contradicts the test intent. Fix by
either deleting the test or converting it to an integration test that runs
against the real DB to verify EXCLUDE_WARMUP_CONDITION actually filters warmup
rows, or if you must keep it as a unit test, change the mock for
findSessionOriginChain to return two rows (first marked warmup, second
non-warmup) and assert that findSessionOriginChain returns the non-warmup chain
(use a non-warmup reason like "initial_selection"/selectionMethod
"initial_selection") so the test actually verifies the skipping behavior.
| "singleSuccess": "{total} 個供應商，{healthy} 個健康 → {provider} ✓", | ||
| "sessionReuse": "會話複用 → {provider} ✓" | ||
| "sessionReuse": "會話複用 → {provider} ✓", | ||
| "originHint": "會話複用 - 最初通過 {method} 選擇" |
There was a problem hiding this comment.
originHint 中的 "通過" 應為繁體中文慣用的 "透過"
"會話複用 - 最初通過 {method} 選擇" 中的「通過」在繁體中文（zh-TW）語境下通常表示「通過審核/法案」，而非「藉由某種方式」。表達 via / by means of 的慣用繁體中文詞彙應為「透過」，否則對母語讀者而言語意略顯歧義。
✏️ 建議修正
- "originHint": "會話複用 - 最初通過 {method} 選擇"
+ "originHint": "會話複用 - 最初透過 {method} 選擇"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "originHint": "會話複用 - 最初通過 {method} 選擇" | |
| "originHint": "會話複用 - 最初透過 {method} 選擇" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@messages/zh-TW/provider-chain.json` at line 23, Update the zh-TW message for
the JSON key originHint to use the customary Traditional Chinese wording for
"via" by replacing "通過" with "透過" in the string "會話複用 - 最初通過 {method} 選擇" so it
becomes "會話複用 - 最初透過 {method} 選擇"; ensure only the wording is changed and the
placeholder {method} remains untouched.
| const trigger = Array.from(container.querySelectorAll("button")).find((button) => | ||
| button.textContent?.includes("View original selection") | ||
| ); | ||
|
|
||
| expect(trigger).toBeTruthy(); | ||
| click(trigger!); | ||
|
|
||
| await act(async () => { | ||
| await Promise.resolve(); | ||
| }); | ||
|
|
||
| expect(getSessionOriginChainMock).toHaveBeenCalledWith("sess-origin-3"); | ||
| expect(getSessionOriginChainMock).toHaveBeenCalledTimes(1); | ||
| expect(container.textContent).toContain("Original decision record unavailable"); | ||
| unmount(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the test file at the specified lines
wc -l "src/app/[locale]/dashboard/logs/_components/error-details-dialog.test.tsx"Repository: ding113/claude-code-hub
Length of output: 145
🏁 Script executed:
# Read the test section around lines 1160-1175
sed -n '1140,1180p' "src/app/[locale]/dashboard/logs/_components/error-details-dialog.test.tsx"Repository: ding113/claude-code-hub
Length of output: 1063
🏁 Script executed:
# Search for the test function containing this code to understand full context
grep -n "View original selection" "src/app/[locale]/dashboard/logs/_components/error-details-dialog.test.tsx"Repository: ding113/claude-code-hub
Length of output: 256
🏁 Script executed:
# Look for getSessionOriginChainMock setup and usage
grep -n "getSessionOriginChainMock\|getSessionOriginChain" "src/app/[locale]/dashboard/logs/_components/error-details-dialog.test.tsx" | head -20Repository: ding113/claude-code-hub
Length of output: 554
🏁 Script executed:
# Find the component implementation to understand the async flow
find . -name "error-details-dialog.tsx" -o -name "*error-details*" -type f | grep -E "\.(tsx|ts)$" | grep -v test | grep -v node_modulesRepository: ding113/claude-code-hub
Length of output: 49
🏁 Script executed:
# Find all files related to error-details-dialog
find . -type f -name "*error-details*" | grep -v node_modules | grep -v ".next"Repository: ding113/claude-code-hub
Length of output: 267
🏁 Script executed:
# Look for the component in src directory
ls -la "src/app/[locale]/dashboard/logs/_components/" 2>/dev/null | head -20Repository: ding113/claude-code-hub
Length of output: 1463
🏁 Script executed:
# Search for ErrorDetailsDialog component definition
rg "export.*ErrorDetailsDialog|function ErrorDetailsDialog|const ErrorDetailsDialog" --type ts --type tsxRepository: ding113/claude-code-hub
Length of output: 94
🏁 Script executed:
# Check how click() function is implemented - might be custom test utility
grep -n "const click\|function click\|import.*click" "src/app/[locale]/dashboard/logs/_components/error-details-dialog.test.tsx" | head -5Repository: ding113/claude-code-hub
Length of output: 112
🏁 Script executed:
# Look for the test setup and how act is being used
sed -n '1,50p' "src/app/[locale]/dashboard/logs/_components/error-details-dialog.test.tsx"Repository: ding113/claude-code-hub
Length of output: 1708
🏁 Script executed:
# Get the click function implementation
sed -n '390,410p' "src/app/[locale]/dashboard/logs/_components/error-details-dialog.test.tsx"Repository: ding113/claude-code-hub
Length of output: 734
🏁 Script executed:
# Check the component directory structure
ls -la "src/app/[locale]/dashboard/logs/_components/error-details-dialog/"Repository: ding113/claude-code-hub
Length of output: 360
🏁 Script executed:
# Get the actual component implementation
find "src/app/[locale]/dashboard/logs/_components/error-details-dialog" -type f -name "*.tsx" -o -name "*.ts"Repository: ding113/claude-code-hub
Length of output: 861
🏁 Script executed:
# Look for the component that handles the getSessionOriginChain call
rg "getSessionOriginChain" --type ts --type tsx -A 5 -B 5 | head -60Repository: ding113/claude-code-hub
Length of output: 94
🏁 Script executed:
# Check how the component uses act and async operations in the actual implementation
sed -n '1100,1180p' "src/app/[locale]/dashboard/logs/_components/error-details-dialog.test.tsx"Repository: ding113/claude-code-hub
Length of output: 2014
🏁 Script executed:
# Read the main component implementation to understand the async flow
cat "src/app/[locale]/dashboard/logs/_components/error-details-dialog/index.tsx"Repository: ding113/claude-code-hub
Length of output: 10322
🏁 Script executed:
# Search for where getSessionOriginChain is called in the component
rg "getSessionOriginChain" --type ts -A 10 -B 5 "src/app/[locale]/dashboard/logs/_components/error-details-dialog/"Repository: ding113/claude-code-hub
Length of output: 4472
🏁 Script executed:
# Check if there are any state updates or effects that handle the result
rg "useState\|useEffect\|setOriginChain\|originChain" --type ts "src/app/[locale]/dashboard/logs/_components/error-details-dialog/"Repository: ding113/claude-code-hub
Length of output: 49
🏁 Script executed:
# Look at the LogicTraceTab component more carefully to see the exact async flow
sed -n '1,150p' "src/app/[locale]/dashboard/logs/_components/error-details-dialog/components/LogicTraceTab.tsx"Repository: ding113/claude-code-hub
Length of output: 5901
🏁 Script executed:
# Check if there are any multiple awaits or nested async operations in the collapsible handler
grep -n "onOpenChange\|getSessionOriginChain\|setOriginChain\|setOriginLoading" "src/app/[locale]/dashboard/logs/_components/error-details-dialog/components/LogicTraceTab.tsx" | head -30Repository: ding113/claude-code-hub
Length of output: 627
🏁 Script executed:
# Search for test flakiness reports or CI issues with this test
git log --oneline --all --grep="error-details-dialog\|View original selection\|sess-origin" 2>/dev/null | head -20Repository: ding113/claude-code-hub
Length of output: 776
🏁 Script executed:
# Check if other similar tests use the same pattern or setTimeout(resolve, 0)
rg "await act.*Promise.resolve\|await act.*setTimeout" --type ts "src/" | head -10Repository: ding113/claude-code-hub
Length of output: 49
🏁 Script executed:
# Let me look more closely at the exact lines being tested and the complete async flow
sed -n '1135,1180p' "src/app/[locale]/dashboard/logs/_components/error-details-dialog.test.tsx"Repository: ding113/claude-code-hub
Length of output: 1263
🏁 Script executed:
# Check the exact promise chain in LogicTraceTab to see if there are nested awaits
sed -n '300,330p' "src/app/[locale]/dashboard/logs/_components/error-details-dialog/components/LogicTraceTab.tsx"Repository: ding113/claude-code-hub
Length of output: 1315
🏁 Script executed:
# Search for React 19 async act documentation or patterns
web_search
React 19 async act Promise.resolve() behavior state updatesRepository: ding113/claude-code-hub
Length of output: 262
🌐 Web query:
React 19 async act flush microtasks Promise.resolve behavior state updates
💡 Result:
What changed / what to rely on in React 19
actis now a React API (import { act } from "react"), not something you should pull fromreact-dom/test-utilsanymore. This was part of the React 19 migration story (and even produced warnings/confusion in late React 18.x). [1][2]- Prefer
await act(async () => { ... })even for “mostly sync” tests. React’s docs explicitly recommend the async form because React’s internal scheduling makes it hard to predict when the sync form is sufficient, and they plan to deprecate/remove the sync form. [1]
Microtasks, Promise.resolve, and why “flush microtasks” is tricky
Promise.resolve(value)creates/returns an already-fulfilled Promise, but microtasks happen when you attach reactions (e.g..then(...)) or whenawaitresumes—those continuations run via the microtask queue. That’s why code that “looks resolved already” can still lead to async timing. (In other words: “resolved promise” ≠ “everything ran”.) [3]- The common “flush promises” helper
await Promise.resolve()(or similar) does not guarantee you drained everything if new Promises/microtasks get queued during the flush; it only advances the queue in a limited way. This is why “justawait Promise.resolve()” can be flaky for React state updates triggered through chains of async work. [4]
How async act relates to state updates
- React can schedule some updates “asynchronously” from the test’s point of view (often effectively behind microtasks / internal queues). Testing helpers (
act,waitFor, RTL utilities) exist to ensure React has processed those updates before you assert. A good mental model is:awaitwaits for your async;actwaits for React’s async work triggered by it. [5] - React’s direction (discussed by the team) is that
actworks by routing React’s scheduled work into a queue thatactcan flush deterministically, rather than relying on ad-hoc “await a microtask” hacks. That’s why “flush microtasks withawait null/Promise.resolve()” is not the right primitive to depend on long-term. [3]
Practical guidance (React 19)
- If a state update can happen after any async boundary (promise,
await, Suspense, timers, etc.), use:This is the behavior React documents and is the most future-proof. [1]await act(async () => { // trigger the thing (render, event, resolve a deferred, etc.) });
Sources: React act docs [1]; React issue about importing act from react [2]; React team discussion on act/microtask hacks [3]; microtask flushing limitations with Promise.resolve-style helpers [4]; explanation of why await alone isn’t enough vs act/React’s internal queueing [5].
🌐 Web query:
React Testing Library act async microtask queue flushing cross boundary
💡 Result:
In React tests, act() only guarantees that React’s own work triggered during the act scope (renders, effects, state updates scheduled by React) is flushed before you assert—it does not magically drain “whatever might happen later” in the JS microtask queue once you’ve crossed the act boundary. (react.dev)
What “cross-boundary microtasks” usually means
You do something inside act (or via RTL helpers that wrap act), but the component schedules more async work (often Promise/queueMicrotask chains) that continues after the awaited thing you wrapped—so assertions run while more microtasks are still pending, or you get “not wrapped in act(...)” warnings.
Preferred fix in React Testing Library (RTL)
Don’t fight microtasks directly—await the UI condition with RTL async utilities (they retry until the DOM matches), e.g. findBy... (which is getBy... + waitFor) or waitFor. (testing-library.com)
render(<MyComponent />)
userEvent.click(screen.getByRole('button', { name: /save/i }))
expect(await screen.findByText(/saved/i)).toBeInTheDocument()If you truly must “flush microtasks”
Use a “next macrotask” flush helper (common pattern: setTimeout(0) in Jest/jsdom) to let pending promise microtasks run before continuing—but note this may still miss microtasks scheduled by microtasks (you may need to loop). (stackoverflow.com)
const flushPromises = () => new Promise<void>(r => setTimeout(r, 0))
await act(async () => {
doSomethingThatQueuesPromises()
await flushPromises()
})Extra note
RTL already wraps render/fireEvent in act by default; act is usually only needed when you trigger updates outside RTL’s wrapped APIs. (testing-library.com)
Citations:
- 1: https://react.dev/warnings/react-dom-test-utils?utm_source=openai
- 2: https://testing-library.com/docs/dom-testing-library/api-async?utm_source=openai
- 3: https://stackoverflow.com/questions/61690243/flushing-a-promise-queue?utm_source=openai
- 4: https://testing-library.com/docs/react-testing-library/faq/?utm_source=openai
改用更健壮的异步等待方式，并将点击操作纳入 act 作用域
当前代码在两个方面存在潜在的异步时序风险：
-
click()在act外部执行：点击事件触发的状态更新开始于 React 的调度边界之外，后续的await act(...)只能部分地追踪这些副作用。 -
单一
Promise.resolve()不足以可靠地刷新承诺链：虽然此处的组件代码中.then()/.catch()/.finally()内部没有嵌套的await，但根据 React 19 官方指导，单次Promise.resolve()无法保证在复杂承诺链场景下完全清空微任务队列。长期看，这种模式容易在代码演进后出现偶发失败。
🛡️ 更健壮的异步等待方式
- expect(trigger).toBeTruthy();
- click(trigger!);
-
- await act(async () => {
- await Promise.resolve();
- });
+ expect(trigger).toBeTruthy();
+ await act(async () => {
+ click(trigger!);
+ await new Promise<void>((resolve) => setTimeout(resolve, 0));
+ });这样做可以确保：
- 点击和后续异步操作都在 React 的统一调度范围内
setTimeout(resolve, 0)允许所有待处理的微任务完成后再继续，而非仅推进单个 tick
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const trigger = Array.from(container.querySelectorAll("button")).find((button) => | |
| button.textContent?.includes("View original selection") | |
| ); | |
| expect(trigger).toBeTruthy(); | |
| click(trigger!); | |
| await act(async () => { | |
| await Promise.resolve(); | |
| }); | |
| expect(getSessionOriginChainMock).toHaveBeenCalledWith("sess-origin-3"); | |
| expect(getSessionOriginChainMock).toHaveBeenCalledTimes(1); | |
| expect(container.textContent).toContain("Original decision record unavailable"); | |
| unmount(); | |
| }); | |
| const trigger = Array.from(container.querySelectorAll("button")).find((button) => | |
| button.textContent?.includes("View original selection") | |
| ); | |
| expect(trigger).toBeTruthy(); | |
| await act(async () => { | |
| click(trigger!); | |
| await new Promise<void>((resolve) => setTimeout(resolve, 0)); | |
| }); | |
| expect(getSessionOriginChainMock).toHaveBeenCalledWith("sess-origin-3"); | |
| expect(getSessionOriginChainMock).toHaveBeenCalledTimes(1); | |
| expect(container.textContent).toContain("Original decision record unavailable"); | |
| unmount(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/`[locale]/dashboard/logs/_components/error-details-dialog.test.tsx
around lines 1160 - 1175, Wrap the button click and the subsequent microtask
drain inside a single act block to ensure React tracks all state updates: call
click(trigger!) within an await act(async () => { ... }) and inside that act
await a full microtask/tick drain (e.g., await new Promise(resolve =>
setTimeout(resolve, 0))) instead of a single Promise.resolve(); keep the
existing assertions for getSessionOriginChainMock and container text, and ensure
trigger is truthy before clicking (trigger variable and
getSessionOriginChainMock referenced to locate the test).
1 participant
Summary
For session reuse requests, the decision chain previously only showed
session_reusewith zero-value placeholders. This PR adds on-demand fetching and display of the original provider selection decision from the first request in that session.Fixes #736
Problem
When viewing a session reuse request's decision chain in the logs UI, users could only see "Session Reuse" with minimal context (session age, priority, cost multiplier). The original decision chain that led to the initial provider selection for that session was not visible, making it difficult to understand why a particular provider was chosen.
Related Issues:
Solution
On-demand fetching of the original decision chain from the first non-warmup request in a session:
providerChainby sessionId, using the existingEXCLUDE_WARMUP_CONDITIONChanges
Core Changes
src/repository/message.ts):findSessionOriginChain(sessionId)— queries first non-warmup request'sproviderChainby sessionIdsrc/actions/session-origin-chain.ts):getSessionOriginChain— auth-gated action with admin/user permission scopingsrc/app/[locale]/dashboard/logs/_components/error-details-dialog/components/LogicTraceTab.tsx): Collapsible "Original Selection Decision" section after the session reuse StepCard; fetches on expand (not on dialog open), silent degradation when unavailablesrc/app/[locale]/dashboard/logs/_components/provider-chain-popover.tsx): Small origin hint text in session reuse tooltip showing original selection methodSupporting Changes
logs.details.logicTrace.originDecisionTitle/Desc/Loading/Unavailable/Expandprovider-chain.summary.originHintTesting
Automated Tests
tests/unit/repository/message-origin-chain.test.ts(5 tests)tests/unit/actions/session-origin-chain.test.ts(4 tests)tests/unit/actions/session-origin-chain-integration.test.ts(1 test)error-details-dialog.test.tsx(3 tests) andprovider-chain-popover.test.tsx(2 tests)Manual Testing
Checklist
bun run typecheckpassesbun run lintpasses (1 pre-existing warning unrelated to this PR)Description enhanced by Claude AI
Greptile Summary
This PR adds on-demand fetching and display of the original provider selection decision chain for session reuse requests. Previously, the decision chain for session reuse only showed zero-value placeholders; now users can expand a collapsible section to see the complete origin decision from the first request in that session.
Key Changes
findSessionOriginChain()that queries the first non-warmup request'sproviderChainusing proper SQL parameterization and the existingEXCLUDE_WARMUP_CONDITIONgetSessionOriginChainwith authentication gates and permission scoping (admins bypass ownership checks, non-admins must own at least one request in the session)LogicTraceTabthat lazy-loads the origin chain only when expanded (not on dialog open)ProviderChainPopovertooltip showing the original selection methodTechnical Highlights
✓ Proper auth/permission scoping with admin bypass
✓ Lazy loading for performance optimization
✓ Silent degradation when origin chain unavailable
✓ SQL injection safe (Drizzle ORM parameterization)
✓ Comprehensive test coverage at all layers
✓ Follows existing warmup exclusion patterns
✓ No hardcoded strings, full i18n support
Confidence Score: 5/5
Important Files Changed
findSessionOriginChainquery using proper SQL parameterization and warmup filteringSequence Diagram
sequenceDiagram participant User participant UI as LogicTraceTab participant Action as getSessionOriginChain participant Auth as getSession participant DB as Database participant Repo as findSessionOriginChain User->>UI: Expand "View original selection" UI->>Action: getSessionOriginChain(sessionId) Action->>Auth: Check authentication Auth-->>Action: Session data alt User is Admin Action->>Repo: findSessionOriginChain(sessionId) else User is Non-Admin Action->>DB: Check ownership (userId or key) DB-->>Action: Ownership verified Action->>Repo: findSessionOriginChain(sessionId) end Repo->>DB: SELECT providerChain<br/>WHERE sessionId = ?<br/>AND deletedAt IS NULL<br/>AND (blockedBy IS NULL OR blockedBy != 'warmup')<br/>AND providerChain IS NOT NULL<br/>ORDER BY requestSequence ASC<br/>LIMIT 1 DB-->>Repo: First non-warmup chain Repo-->>Action: ProviderChainItem[] Action-->>UI: {ok: true, data: chain} UI->>User: Display origin decision detailsLast reviewed commit: 3f109f8