Revert "Reset branch to 76ace98 and synchronize with main"#489
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
✨ 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 |
ngoiyaeric
merged commit e2bd753
into
feature/resolution-search-enhancement-img-3118023839244746163
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Main issues are (1) an app/actions.tsx bug-risk from duplicated groupeId declarations, (2) loss of Mapbox NavigationControl lifecycle management which can lead to duplicated controls/leaks, and (3) multiple maintainability regressions (any for multimodal message content, hardcoded MIME types, Date.now() IDs). Additionally, the resolution-search result payload is double-encoded JSON (image as a JSON string), which increases parsing brittleness and migration cost.
Additional notes (2)
- Maintainability |
components/search-related.tsx:1-16
SearchRelated imports unused hooks/utilities after refactor
useEffect, useState, and readStreamableValue are imported but not used in the shown code. While ESLint may or may not catch this depending on config, keeping dead imports is a maintenance smell and makes the component harder to reason about.
- Maintainability |
components/chat-panel.tsx:117-122
You appenddrawnFeaturesinHeaderSearchButton, butChatPanelandFollowupPanelno longer append it. Meanwhilesubmit()still readsdrawnFeaturesforresolution_searchand (elsewhere) seems to have logic that used to pass drawn features toresearcherbut no longer does. Net effect: drawn features are inconsistently available depending on entry point, which is a behavioral regression for map-context queries.
Summary of changes
Summary
This PR is a revert of QCX#485 and rolls back several UI/UX features while also adjusting map-image resolution search to support dual previews.
Key changes
-
Resolution search input/output revamped
app/actions.tsxnow acceptsmapboxFile+googleFile(with a legacyfilefallback), builds a multimodalcontentarray with up to two images, and stores the result image payload asJSON.stringify({ mapbox, google }).components/resolution-image.tsxupdated to render a side-by-side Mapbox vs Google comparison (thumbnail + full dialog view).components/header-search-button.tsxcaptures both a Mapbox canvas preview and a Google Static Maps preview, sending both to the server.
-
UI providers and panels removed
- Removed
UsageToggleProvider,HistoryToggleProvider,HistorySidebar,UsageView, andPurchaseCreditsPopup. components/history.tsxnow owns its ownSheetUI instead of relying on a global context.
- Removed
-
Agent model selection now vision-aware
lib/utils/getModel(requireVision)now chooses vision vs non-vision models for xAI and usesgemini-1.5-profor Google.inquire,querySuggestor,taskManager, andresearchernow detect image presence and callgetModel(true)accordingly.
-
Mapbox navigation control behavior changed
components/map/mapbox-map.tsxadds aNavigationControlon desktop during map init and removes the prior explicit lifecycle management around drawing-mode transitions.
-
Misc UI changes
- Header history toggle click removed; Stripe link replaces usage toggle button; z-index changed.
- Suggestions overlay handling moved higher into
components/chat.tsx.
| // Construct the multimodal content for the user message. | ||
| const contentParts: any[] = [ | ||
| { type: 'text', text: userInput } | ||
| ] | ||
|
|
||
| if (mapboxDataUrl) { | ||
| contentParts.push({ | ||
| type: 'image', | ||
| image: mapboxDataUrl, | ||
| mimeType: 'image/png' | ||
| }) | ||
| } | ||
| if (googleDataUrl) { | ||
| contentParts.push({ | ||
| type: 'image', | ||
| image: googleDataUrl, | ||
| mimeType: 'image/png' | ||
| }) | ||
| } | ||
|
|
||
| const content = contentParts as any | ||
|
|
There was a problem hiding this comment.
any[] for multimodal content parts hides correctness issues
const contentParts: any[] and const content = contentParts as any bypass the CoreMessage['content'] contract. This is exactly the kind of place where subtle mistakes (wrong mimeType, wrong shape, missing fields) will silently ship and only fail at runtime in a model/tool adapter.
You already used CoreMessage['content'] elsewhere in this file; the resolution-search path should do the same.
Suggestion
Type the parts as CoreMessage['content'] (or the specific multimodal union type your ai SDK expects) and avoid any.
Example direction:
const content: CoreMessage['content'] = [{ type: 'text', text: userInput }, ...images]- Build
imagesas a properly typed array of image parts.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| if (mapboxDataUrl) { | ||
| contentParts.push({ | ||
| type: 'image', | ||
| image: mapboxDataUrl, | ||
| mimeType: 'image/png' | ||
| }) | ||
| } | ||
| if (googleDataUrl) { | ||
| contentParts.push({ | ||
| type: 'image', | ||
| image: googleDataUrl, | ||
| mimeType: 'image/png' | ||
| }) | ||
| } |
There was a problem hiding this comment.
Hardcoded mimeType: 'image/png' ignores the actual file type
The Data URL is created using mapboxFile.type / googleFile.type, but the message parts always send mimeType: 'image/png'. If the browser provides e.g. image/jpeg or image/webp, downstream model/tooling could mis-handle or reject the payload.
This is a runtime correctness issue, not a typing issue.
Suggestion
Preserve the original MIME type alongside each data URL. For example, store mapboxMimeType / googleMimeType when building the data URLs and pass them through into the content parts.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| role: 'assistant', | ||
| content: JSON.stringify({ | ||
| ...analysisResult, | ||
| image: dataUrl | ||
| image: JSON.stringify({ mapbox: mapboxDataUrl, google: googleDataUrl }) | ||
| }), | ||
| type: 'resolution_search_result' | ||
| }, |
There was a problem hiding this comment.
Resolution search stores image as a JSON string inside an already JSON-stringified message
You’re doing content: JSON.stringify({ ...analysisResult, image: JSON.stringify({ mapbox, google }) }). That means the image field is double-encoded, forcing consumers to JSON.parse twice and complicating backward compatibility.
This increases brittleness and makes migrations harder (you already had to add a try/catch parsing branch).
Suggestion
Store image as an object directly in the assistant message payload: image: { mapbox: mapboxDataUrl, google: googleDataUrl }. Then adjust the UI-state parser to accept either an object (new) or string (old).
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| const handleSubmit = async (event: React.FormEvent<HTMLFormElement>) => { | ||
| event.preventDefault() | ||
| const formData = new FormData(event.currentTarget as HTMLFormElement) | ||
|
|
||
| // // Get the submitter of the form | ||
| const submitter = (event.nativeEvent as SubmitEvent) | ||
| .submitter as HTMLInputElement | ||
| let query = '' | ||
| if (submitter) { | ||
| formData.append(submitter.name, submitter.value) | ||
| query = submitter.value | ||
| } | ||
|
|
||
| const userMessage = { | ||
| id: nanoid(), | ||
| id: Date.now(), | ||
| component: <UserMessage content={query} /> | ||
| } |
There was a problem hiding this comment.
SearchRelated switched IDs from nanoid() to Date.now() which can collide
Date.now() is not a safe unique identifier in fast, repeated interactions (multiple clicks within the same millisecond are possible, especially in tests or on fast devices). The rest of the codebase already used nanoid() for this.
Colliding IDs can break React list keys and message state updates.
Suggestion
Use nanoid() (or a deterministic stable key derived from content + index) instead of Date.now() for message IDs.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
1 participant
User description
Reverts #485
PR Type
Enhancement, Bug fix
Description
Update AI model selection to use vision-capable models when images present
Support dual map provider imagery (Mapbox and Google) in resolution search
Remove unused UI components (usage view, history sidebar context)
Simplify provider context hierarchy and remove MCP-related code
Fix map navigation controls and drawing mode interactions
Diagram Walkthrough
File Walkthrough
21 files
Update model selection for vision capabilitySupport dual map imagery and refactor resolution searchRemove unused context providers from layoutRemove MCP instance and simplify form handlingRemove usage view and simplify suggestions renderingRemove usage toggle from visibility logicRemove MCP and drawn features from form dataCapture both Mapbox and Google map previewsRemove purchase popup and usage toggle functionalityRefactor history to use Sheet component directlyUpdate Stripe purchase linkRemove closeProfileView function from contextRemove usage toggle interaction logicSupport dual image display for map comparisonConvert to form-based submission and remove MCPRemove credits preview and history toggle dependencyAdd vision model detection for image contentAdd vision model detection for image contentRemove drawn features from system promptUpdate system prompt for dual image analysisAdd vision model detection for image content1 files
Remove drawing mode zoom control test6 files
Delete license document fileDelete history sidebar component fileDelete history toggle context providerDelete purchase credits popup componentDelete usage toggle context providerDelete usage view component file1 files
Simplify navigation control and drawing mode logic