refactor(ai-assistant): use smart doc sse parser#323
refactor(ai-assistant): use smart doc sse parser#323yangxiaolang wants to merge 4 commits intomainfrom
Conversation
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
WalkthroughReplaces local SmartDoc/SSE parsing and streaming delta logic with an external SmartDoc SSE parser; updates types, components, styles, and tests to consume the parser's structured display messages and new message fields. Also adds the parser dependency to package.json. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 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 |
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #323 +/- ##
==========================================
- Coverage 53.20% 52.32% -0.89%
==========================================
Files 61 61
Lines 1528 1462 -66
Branches 483 462 -21
==========================================
- Hits 813 765 -48
+ Misses 548 538 -10
+ Partials 167 159 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/doom/test/global/Intelligence/AIAssistant/utils.spec.ts (1)
10-86: Consider scoping tests to our own wrapper rather than re-verifying library internals.The
SmartDocSseParser,parseSmartDocMessage, andformatSmartDocMarkdowndescribe blocks assert library behavior directly (trace-tag replacement, metadata filtering, markdown formatting rules, exact JSON field ordering in snapshots). The library owns these invariants and their tests; mirroring them here couples this test suite to implementation details of@yangxiaolang/smart-doc-sse-parser, so library-internal refactors break tests without a local code change.Consider trimming these down to:
parseStreamContentcontract (already covered well at lines 88–114).normalizeRefDocsdefaults (partial/missing fields → defaulted shape).- Any wrapper-specific edge cases (incomplete tag stripping, fallback to
message.result).If you want a smoke test that the library import is wired correctly, a single
expect(typeof SmartDocSseParser).toBe('function')is usually enough.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/doom/test/global/Intelligence/AIAssistant/utils.spec.ts` around lines 10 - 86, The tests are tightly coupled to `@yangxiaolang/smart-doc-sse-parser` internals (SmartDocSseParser, parseSmartDocMessage, formatSmartDocMarkdown) and should be scoped to our wrapper behavior: replace detailed library-internal assertions (trace-tag replacement, metadata filtering, exact JSON field ordering) with a simple smoke check for the import (e.g., expect(typeof SmartDocSseParser) toBe('function')) and add focused tests for our wrapper contracts — ensure parseStreamContent behavior, normalizeRefDocs defaults (missing/partial fields produce the normalized shape), and any wrapper-specific edge cases (incomplete tag stripping, fallback to message.result) are covered; remove or trim assertions that re-verify library formatting logic and instead assert only the wrapper’s transformations and defaults.packages/doom/src/global/Intelligence/AIAssistant/utils.ts (1)
50-54: UnusedthoughtProcessassignment onnormalizedMessage.Only
normalizedMessage.refDocs(line 56) andnormalizedMessage.result(line 58) are consumed, so spreading...messageand overridingthoughtProcessis dead work. You can simplify and drop the spread.♻️ Suggested simplification
-export function parseStreamContent(text: string) { - const message = parseSmartDocMessage(text) - const thinkingProcess = getThinkingProcess(text, message) - const result = getDisplayResult(text, message) - const normalizedMessage = { - ...message, - result, - thoughtProcess: thinkingProcess ?? '', - } - return { - refDocs: normalizeRefDocs(normalizedMessage.refDocs), - thinkingProcess, - content: formatSmartDocMarkdown(normalizedMessage.result), - } -} +export function parseStreamContent(text: string) { + const message = parseSmartDocMessage(text) + const thinkingProcess = getThinkingProcess(text, message) + const result = getDisplayResult(text, message) + return { + refDocs: normalizeRefDocs(message.refDocs), + thinkingProcess, + content: formatSmartDocMarkdown(result), + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/doom/src/global/Intelligence/AIAssistant/utils.ts` around lines 50 - 54, The code unnecessarily spreads the entire message object and overrides the thoughtProcess property in normalizedMessage, but only refDocs and result properties are used later. To fix this, replace the spread of message with only the needed properties refDocs and assign result directly, removing the thoughtProcess assignment altogether in the normalizedMessage object construction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/doom/package.json`:
- Line 63: The dependency "@yangxiaolang/smart-doc-sse-parser" is a
personal-scoped package used by the runtime; mitigate supply-chain risk by
either 1) publishing it under the org scope (e.g., move package and its repo to
`@alauda` and update the dependency ref), 2) vendoring the parser into this
monorepo as an internal workspace package (add it under packages/ and change the
dependency to the local workspace name), or 3) if you cannot move/vendor now,
add a strict lock via package.json "resolutions" (or npm/yarn lockfile policy)
and a security policy note documenting the pinned 0.3.0 usage; update
package.json to reflect whichever approach and ensure build/test scripts
reference the new package name/location (look for the dependency entry
"@yangxiaolang/smart-doc-sse-parser" to change).
In `@packages/doom/src/global/Intelligence/AIAssistant/utils.ts`:
- Around line 29-30: The current greedy fallback regex openThinkMatch =
text.match(/<think>([\s\S]*)$/) can over-capture across closed </think> blocks;
replace it with a defensive global non-greedy extraction using
/<think>([\s\S]*?)(?:<\/think>|$)/g and pick the last match (e.g., via
Array.from(text.matchAll(...)).pop()) so you capture only the content of the
last open <think> without including intervening </think> tags; update the code
that reads openThinkMatch?.[1] accordingly and ensure this runs only when
message.thoughtProcess is empty.
---
Nitpick comments:
In `@packages/doom/src/global/Intelligence/AIAssistant/utils.ts`:
- Around line 50-54: The code unnecessarily spreads the entire message object
and overrides the thoughtProcess property in normalizedMessage, but only refDocs
and result properties are used later. To fix this, replace the spread of message
with only the needed properties refDocs and assign result directly, removing the
thoughtProcess assignment altogether in the normalizedMessage object
construction.
In `@packages/doom/test/global/Intelligence/AIAssistant/utils.spec.ts`:
- Around line 10-86: The tests are tightly coupled to
`@yangxiaolang/smart-doc-sse-parser` internals (SmartDocSseParser,
parseSmartDocMessage, formatSmartDocMarkdown) and should be scoped to our
wrapper behavior: replace detailed library-internal assertions (trace-tag
replacement, metadata filtering, exact JSON field ordering) with a simple smoke
check for the import (e.g., expect(typeof SmartDocSseParser) toBe('function'))
and add focused tests for our wrapper contracts — ensure parseStreamContent
behavior, normalizeRefDocs defaults (missing/partial fields produce the
normalized shape), and any wrapper-specific edge cases (incomplete tag
stripping, fallback to message.result) are covered; remove or trim assertions
that re-verify library formatting logic and instead assert only the wrapper’s
transformations and defaults.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b4d9964-1335-48a1-b1b4-4404093652f5
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
packages/doom/package.jsonpackages/doom/src/global/Intelligence/AIAssistant/index.tsxpackages/doom/src/global/Intelligence/AIAssistant/utils.tspackages/doom/test/global/Intelligence/AIAssistant/utils.spec.ts
… consumeSmartDocDisplayStream Replace manual SSE parsing logic with consumeSmartDocDisplayStream API from the library. Remove local utils.ts wrapper, rename thinkingProcess to thoughtProcess, use SmartDocDisplayReferenceDoc type, and add markdown content styling.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/doom/src/global/Intelligence/AIAssistant/index.tsx (1)
142-161: Minor:thoughtProcessparameter typing.
nextMessage.thoughtProcessis declaredstring, butChatMessage.thoughtProcessisstring | nulland the body usesnextMessage.thoughtProcess || undefined— suggesting the parser may emitnull/empty. Consider widening tostring | null(or inferring directly from the parser's display-message type) for consistency with the underlying field.Suggested tweak
const syncAssistantMessage = (nextMessage: { content: string - thoughtProcess: string + thoughtProcess: string | null refDocs: ChatMessage['refDocs'] }) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/doom/src/global/Intelligence/AIAssistant/index.tsx` around lines 142 - 161, The parameter typing for syncAssistantMessage's nextMessage currently declares thoughtProcess as string but the underlying ChatMessage.thoughtProcess is string | null and the code uses nextMessage.thoughtProcess || undefined; update the type of nextMessage.thoughtProcess to string | null (or use the parser/display-message type that already defines thoughtProcess) so the signature matches ChatMessage and avoids inconsistent null/undefined handling in syncAssistantMessage and the flushMessages update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/doom/src/global/Intelligence/AIAssistant/index.tsx`:
- Around line 142-161: The parameter typing for syncAssistantMessage's
nextMessage currently declares thoughtProcess as string but the underlying
ChatMessage.thoughtProcess is string | null and the code uses
nextMessage.thoughtProcess || undefined; update the type of
nextMessage.thoughtProcess to string | null (or use the parser/display-message
type that already defines thoughtProcess) so the signature matches ChatMessage
and avoids inconsistent null/undefined handling in syncAssistantMessage and the
flushMessages update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cdf241d5-50fb-4e17-8ea3-d89c822ee46b
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
packages/doom/package.jsonpackages/doom/src/global/Intelligence/AIAssistant/Chat/ChatRefDocs/index.tsxpackages/doom/src/global/Intelligence/AIAssistant/Chat/ThinkingProcess/index.tsxpackages/doom/src/global/Intelligence/AIAssistant/Chat/index.tsxpackages/doom/src/global/Intelligence/AIAssistant/index.tsxpackages/doom/src/global/Intelligence/AIAssistant/types.tspackages/doom/src/global/Intelligence/AIAssistant/utils.tspackages/doom/styles/chat.module.scss
💤 Files with no reviewable changes (1)
- packages/doom/src/global/Intelligence/AIAssistant/utils.ts
✅ Files skipped from review due to trivial changes (2)
- packages/doom/package.json
- packages/doom/src/global/Intelligence/AIAssistant/Chat/ChatRefDocs/index.tsx
| border-bottom: 0; | ||
| } | ||
|
|
||
| p { |
There was a problem hiding this comment.
这些 CSS 有必要覆盖吗？跟主站点就不一致了。这个是什么考虑？
There was a problem hiding this comment.
这块是想让 AIAssistant 这边的 md 整体紧凑一点
| parseSmartDocMessage, | ||
| parseStreamContent, | ||
| SmartDocSseParser, | ||
| } from '#global/Intelligence/AIAssistant/utils.ts' |
2 participants
Summary by CodeRabbit
Dependencies
Refactor
Tests
Style / UI