refactor(wiki-compose): apply thermo-nuclear code quality review fixes - #992
Conversation
- Align frontend backend resolution with Google-fixed Wiki Compose model - Extract session reducer module; slim useWikiComposeSession hook - Consolidate auto-start via startPolicy and canRetryStart - Deduplicate compose session locale prep on API routes - Remove duplicate props and dead outline preview ternary Co-authored-by: Akimasa Sugai <otomatty@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughこのPRはサーバーでのcontent-locale準備を集中化し、クライアント側でセッションUIの純粋なリデューサ群を導入、バックエンド選定に許容リスト／強制ルールを追加し、フックとページをポリシー駆動の起動制御へ書き換えます。 ChangesCompose Session Locale and State Management Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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 docstrings
Comment |
![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 refactors the Wiki Compose session logic by extracting pure state reduction into a separate reducer (wikiComposeSessionReducer.ts), introducing a startPolicy configuration to manage session auto-start behavior, and restricting allowed execution backends to zedi_managed and user_google. It also extracts shared locale preparation helpers on the server side. Feedback from the review highlights two issues: first, the canRetryStart condition is overly restrictive and prevents retry actions under the default on-mount policy; second, a potential runtime error exists in the reducer when mapping c.sections without validating that each element is a valid object.
| const canRetryStart = | ||
| startPolicy === "when-backend-ready" && | ||
| !initialSessionId && | ||
| Boolean(state.error) && | ||
| !state.session && | ||
| !state.isStreaming && | ||
| (state.status === "idle" || state.status === "failed"); |
There was a problem hiding this comment.
canRetryStart が startPolicy === "when-backend-ready" に制限されているため、デフォルトの startPolicy である "on-mount" の場合に初期起動（セッション作成や初回ストリーム開始）が失敗した際、画面に再試行（Retry）ボタンが表示されなくなってしまいます。
startPolicy に関わらず、新規セッション作成（!initialSessionId）の自動起動が失敗した場合は手動で再試行できるように、この条件を削除することをお勧めします。
| const canRetryStart = | |
| startPolicy === "when-backend-ready" && | |
| !initialSessionId && | |
| Boolean(state.error) && | |
| !state.session && | |
| !state.isStreaming && | |
| (state.status === "idle" || state.status === "failed"); | |
| const canRetryStart = | |
| !initialSessionId && | |
| Boolean(state.error) && | |
| !state.session && | |
| !state.isStreaming && | |
| (state.status === "idle" || state.status === "failed"); |
| } else if (c.sections.length > 0) { | ||
| partial.outlineProposal = c.sections.map((s) => ({ | ||
| id: s.sectionId, | ||
| heading: s.heading, | ||
| depth: 1, | ||
| intent: "", | ||
| })); | ||
| } |
There was a problem hiding this comment.
c.sections の各要素に対するループ処理（398行目付近）では !section || typeof section !== "object" による安全なオブジェクトチェックが行われていますが、こちらの map 処理（409行目付近）ではチェックなしで直接プロパティ（s.sectionId や s.heading）にアクセスしています。
万が一 c.sections に null や非オブジェクトの要素が含まれていた場合、ランタイムエラー（TypeError: Cannot read properties of null）が発生する可能性があります。
直前のループと同様に、事前にフィルタリングを行うか、安全なアクセスを行うように修正することをお勧めします。
} else if (c.sections.length > 0) {
partial.outlineProposal = c.sections
.filter((s): s is DraftedSection => Boolean(s && typeof s === "object" && s.sectionId))
.map((s) => ({
id: s.sectionId,
heading: s.heading ?? "",
depth: 1,
intent: "",
}));
}
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02fb48eff9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cancelled = true; | ||
| abortRef.current?.abort(); |
There was a problem hiding this comment.
For fresh compose (startPolicy === "when-backend-ready", as used by WikiComposePage) start() creates the session, updates state.session, then opens streamRun; that state update flips awaitingFreshStart from true to false, which is in this effect's dependency list, so React runs this cleanup and aborts the just-opened SSE request. The server treats that client disconnect as a failed run, so newly created compose sessions can be cancelled/failed immediately after they start rather than reaching the first interrupt.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/api/src/routes/composeSessionRunLocale.ts (1)
12-17: ⚡ Quick winAdd Japanese alongside English in exported type docs.
ComposeRunLocalePrepdocumentation is currently English-only; please make this exported API doc bilingual to match repository convention.Suggested update
-/** Result of preparing a `POST /run` request for LangGraph execution. */ +/** + * Result of preparing a `POST /run` request for LangGraph execution. + * LangGraph 実行向け `POST /run` リクエスト前処理の結果。 + */ export type ComposeRunLocalePrep = { contentLocale: ComposeContentLocale; graphInput: unknown; - /** Metadata blob to persist on claim when locale is not yet stored. */ + /** + * Metadata blob to persist on claim when locale is not yet stored. + * ロケール未保存時に claim 更新で永続化するメタデータ。 + */ metadataUpdate: Record<string, unknown> | undefined; };As per coding guidelines
**/*.{ts,tsx,js,jsx,md}: Include both Japanese and English comments/documentation in code and documentation files to maintain project tone consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/api/src/routes/composeSessionRunLocale.ts` around lines 12 - 17, Update the JSDoc for the exported type ComposeRunLocalePrep to include both English and Japanese descriptions: keep the existing English sentence for the type and add a concise Japanese equivalent, and do the same for the field comments (contentLocale, graphInput, metadataUpdate) so each comment has an English line followed by a Japanese translation; locate the type declaration for ComposeRunLocalePrep and insert the Japanese translations directly under or inline with the existing English docs to follow the bilingual comment convention.server/api/src/__tests__/routes/composeSessionRunLocale.test.ts (1)
1-3: ⚡ Quick winMake spec-facing test text bilingual (EN/JA).
Since these tests encode behavior specs, keep their documentation/title strings bilingual for consistency with project tone.
As per coding guidelines
**/*.{test,spec}.{ts,tsx,js,jsx}: Tests serve as a source of truth for specifications alongside implementation code TSDoc/JSDoc, and**/*.{ts,tsx,js,jsx,md}: Include both Japanese and English comments/documentation in code and documentation files to maintain project tone consistency.Also applies to: 10-44
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/api/src/__tests__/routes/composeSessionRunLocale.test.ts` around lines 1 - 3, Update the spec-facing test titles and any top-level describe/it strings in the composeSessionRunLocale.test.ts unit tests to include both English and Japanese text (bilingual), e.g., prepend or append Japanese translations to existing English descriptions; locate and edit the describe/it/test blocks and any comment header in this file to ensure each spec string contains an English sentence and its Japanese counterpart so the tests’ documentation/readme tone is bilingual and consistent with project guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/api/src/__tests__/routes/composeSessionRunLocale.test.ts`:
- Around line 1-3: Update the spec-facing test titles and any top-level
describe/it strings in the composeSessionRunLocale.test.ts unit tests to include
both English and Japanese text (bilingual), e.g., prepend or append Japanese
translations to existing English descriptions; locate and edit the
describe/it/test blocks and any comment header in this file to ensure each spec
string contains an English sentence and its Japanese counterpart so the tests’
documentation/readme tone is bilingual and consistent with project guidelines.
In `@server/api/src/routes/composeSessionRunLocale.ts`:
- Around line 12-17: Update the JSDoc for the exported type ComposeRunLocalePrep
to include both English and Japanese descriptions: keep the existing English
sentence for the type and add a concise Japanese equivalent, and do the same for
the field comments (contentLocale, graphInput, metadataUpdate) so each comment
has an English line followed by a Japanese translation; locate the type
declaration for ComposeRunLocalePrep and insert the Japanese translations
directly under or inline with the existing English docs to follow the bilingual
comment convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ba29ee6a-8166-4ca8-adc4-168e9d780eac
📒 Files selected for processing (9)
server/api/src/__tests__/routes/composeSessionRunLocale.test.tsserver/api/src/routes/composeSessionRunLocale.tsserver/api/src/routes/composeSessions.tssrc/hooks/useWikiComposeSession.test.tssrc/hooks/useWikiComposeSession.tssrc/lib/wikiCompose/resolveComposeBackend.test.tssrc/lib/wikiCompose/resolveComposeBackend.tssrc/lib/wikiCompose/wikiComposeSessionReducer.tssrc/pages/WikiComposePage.tsx
- Stop aborting SSE when session state updates after auto-start (Codex P1) - Allow canRetryStart for any fresh-compose auto-start failure - Guard outline mapping in resume reducer against invalid sections - Add bilingual JSDoc on ComposeRunLocalePrep Co-authored-by: Akimasa Sugai <otomatty@users.noreply.github.com>
server/api's vitest suite (129 files / ~1,400 tests) was never run in CI — only api-typecheck (`bunx tsc --noEmit`) ran, so a broken suite could slip through unnoticed (e.g. the bun:test import in #992). server/hocuspocus had the same gap: its tests were only wired into the root `test:run` script, which CI does not execute (CI runs `test:coverage`). Add `api-test` and `hocuspocus-test` jobs mirroring the `mcp-test` setup (install deps inside the service dir, then `vitest run`), and align the misleading comment in the `test` job. Update AGENTS.md so the CI description matches reality, enumerating which job covers each service. Closes #1010 Co-authored-by: Claude <noreply@anthropic.com>
2 participants
概要
Thermo-nuclear コード品質レビューで挙がった推奨アクションをすべて反映しました。Wiki Compose の backend 契約の整合、フックの分解、auto-start の一本化、API の locale 準備の共通化、コピペ残骸の削除です。
変更点
coerceWikiComposeBackend/isWikiComposeAllowedBackendを追加し、非 Google BYOK 設定時はuser_googleまたはzedi_managedにフォールバック（サーバ#990と整合）wikiComposeSessionReducer.ts: SSE / resume / projection の純関数 reducer を切り出し（useWikiComposeSessionは約 300 行に）startPolicy（on-mount/when-backend-ready/never）とcanRetryStartをフックに集約し、WikiComposePageの ref/effect を削除composeSessionRunLocale.tsでPOST /runの locale 解決・metadata 更新・graph input 整形を共通化submitConflictAck/onSubmitConflictAckの重複、無意味なoutlineForPreview三項演算子を削除変更の種類
テスト方法
bunx vitest run src/lib/wikiCompose/resolveComposeBackend.test.ts src/hooks/useWikiComposeSession.test.tscd server/api && bun test src/__tests__/routes/composeSessionRunLocale.test.tszedi_managedまたはuser_googleで開始されることチェックリスト
.ja.mdペア）Summary by CodeRabbit
New Features
Refactor
Tests