Repository navigation
refactor: DRY model tier detection, tool merging, and log cleanup - #685
Conversation
Centralize hardcoded model ID checks (`=== 'glm-4.7'`) behind `getPageSpaceModelTier()` across chat route, global assistant route, and SidebarSettingsTab. Extract `mergeToolSets()` utility to eliminate `eslint-disable` / `as any` casts for tool merging. Unify duplicated message reconstruction logic via shared `reconstructFromStructuredContent` helper (-70 lines). Replace `console.error` with structured `loggers` calls, strip emoji prefixes from global route log messages to match chat route conventions, and remove deprecated `StreamingMarkdown` id prop plus unused variables. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughCentralizes PageSpace model-tier determination and tool merging (getPageSpaceModelTier, mergeToolSets), removes deprecated Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as Chat API Route
participant Utils as Tier/Tool Utils
participant Provider as AI Provider
participant DB as Database
Client->>API: Send chat request (model, tools, message)
API->>Utils: getPageSpaceModelTier(currentModel)
API->>Utils: mergeToolSets(filteredTools, integrationTools)
API->>Provider: Forward request (providerType based on tier, merged tools)
Provider-->>API: Streaming/response (tool calls/results)
API->>DB: Save message (use reconstructFromStructuredContent)
DB-->>API: Ack saved
API-->>Client: Streamed response / final message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
![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
🤖 Fix all issues with AI agents
In
`@apps/web/src/components/layout/right-sidebar/ai-assistant/SidebarSettingsTab.tsx`:
- Around line 647-649: The Upgrade CTA condition in SidebarSettingsTab.tsx
currently checks hasModelAccess('pagespace', PAGESPACE_MODEL_ALIASES.standard)
which depends on the earlier inverted requiresSubscription logic; update this
conditional to check the correct gating model (use PAGESPACE_MODEL_ALIASES.pro
instead of .standard) so the CTA shows only when the user truly lacks pro
access, and verify the hasModelAccess('pagespace', ...) call and any related
requiresSubscription inversion are consistent across the component.
- Around line 264-266: The subscription gating is inverted: update the
requiresSubscription function to return true only for the pro tier (change the
check in requiresSubscription(provider, model) from getPageSpaceModelTier(model)
=== 'standard' to === 'pro') and then update the related logic that resets or
blocks free users (the code at the places that handle selected model for free
users—the reset logic around the selection at the earlier block that currently
targets 'standard'—to target 'pro' instead) and any CTA/visibility checks (the
check that shows the upgrade CTA which currently treats 'standard' as
restricted) so they consistently treat 'pro' as the restricted tier.
🧹 Nitpick comments (3)
apps/web/src/components/ai/shared/chat/CompactMessageRenderer.tsx (1)
129-404: Significant duplication withMessageRenderer.tsx.
CompactMessageRendererandMessageRenderershare nearly identical logic for todo list state management, socket handling, task loading, editing, deletion, and retry — differing only in styling/layout. The PR title mentions DRY cleanup; this is a prime candidate for extracting shared logic into a custom hook (e.g.,useMessageState) to centralize the duplicated state, effects, and handlers across both renderers.apps/web/src/lib/ai/core/message-utils.ts (1)
348-356: Consider inlining the trivial wrapper functions.Both
reconstructMessageFromStructuredContentandreconstructGlobalAssistantMessageFromStructuredContentare now single-line delegates toreconstructFromStructuredContent. Since they're private (not exported) and each has exactly one call site (lines 247 and 490 respectively), they could be inlined to reduce indirection.♻️ Suggested simplification
In
convertDbMessageToUIMessage(line 247):- return reconstructMessageFromStructuredContent(dbMessage, parsed); + return reconstructFromStructuredContent(dbMessage, parsed);In
convertGlobalAssistantMessageToUIMessage(line 490):- return reconstructGlobalAssistantMessageFromStructuredContent(dbMessage, parsed); + return reconstructFromStructuredContent(dbMessage, parsed);Then remove both wrapper functions (lines 348–356 and 512–517).
Also applies to: 512-517
apps/web/src/lib/ai/core/tool-utils.ts (1)
1-5: Centralizing the cast is a good DRY improvement.One consideration: the
additional: Record<string, unknown>parameter accepts anything, so theas ToolSetcast silently trusts the caller. If integration tool shapes ever diverge from whatToolSetexpects, errors will surface at runtime rather than compile time.If the Vercel AI SDK's
ToolSetdefinition allows it, consider tighteningadditionalto something likeRecord<string, ToolSet[string]>(or a partial tool shape) so the compiler can catch mismatches. If the current type is intentional because integration tool shapes don't conform, this is fine as-is.
The requiresSubscription check was gating 'standard' tier instead of 'pro', blocking free users from the standard model and showing the upgrade CTA for the wrong tier. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
=== 'glm-4.7',=== 'glm-5') behindgetPageSpaceModelTier()across chat route, global assistant route,SidebarSettingsTab, andgetUserFacingModelNamemergeToolSets()utility to eliminateeslint-disable/as anycasts for integration tool mergingreconstructFromStructuredContenthelper (−70 lines)console.errorwith structuredloggerscalls in message-utilsStreamingMarkdownidprop, unused_isPullingvar, and backward-compat testVirtualizedMessageListmeasurement fromsetTimeout(50)torequestAnimationFrameTest plan
ai-providers-config.test.ts— 24/24 passStreamingMarkdown.test.tsx— 14/14 passpnpm typecheckbuild — 10/10 tasks pass🤖 Generated with Claude Code
Summary by CodeRabbit
Performance Improvements
Refactor
Tests