Reset branch to commit bf6043b #498
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
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 |
|
|
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan 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 |
|||||||||||||||||||||||||
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan 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.
This reset reintroduces several regressions and reliability/security risks: a likely router.refresh() loop in Chat, mobile search becoming non-functional due to portal removal, and provider-incompatible search button disabling logic. The drawing tool and schema have been weakened with any casts and reduced validation, increasing runtime failures and type drift (especially after deleting lib/types/tools.ts). Additionally, MCP utilities now log partial secrets (security issue) and Google static map URL generation no longer fails fast when misconfigured.
Additional notes (7)
-
Maintainability |
components/chat.tsx:76-83
router.refresh()is now triggered whenever the last AI message is aresponse, but the effect depends on the entireaiStateobject. Ifrouter.refresh()causesaiStateto be re-instantiated (or causes repeated server re-renders that rehydrate state), this can re-trigger the effect repeatedly and create a refresh loop. The previous code used alastRefreshedMessageIdRefguard, which was specifically preventing this class of bug. -
Performance |
components/chat.tsx:94-101
This effect calls a server action every timedrawnFeaturesorcameraStatechanges, and also logs full feature payloads. Drawing interactions can update state at high frequency, which can spam the server, increase costs, and degrade UX. It also risks racing updates (out-of-order writes) and producing inconsistent persisted context. -
Compatibility |
components/header-search-button.tsx:105-105
The disabled logic now uses!mapfor all providers, but earlier logic allowed Google Maps to work based onmapData.cameraState. As written, the search button will be disabled for Google Maps even when the camera state is available (becausemaplikely refers to a Mapbox instance). That’s a behavioral regression. -
Readability |
components/header-search-button.tsx:33-33
Removing theMutationObserverportal discovery means portals will only be found if the portal nodes exist at the time this component mounts. If those portal targets are rendered later (common with conditional headers/layouts), the search button will never render.
The deleted observer logic was there to handle exactly that.
-
Compatibility |
components/mobile-icons-bar.tsx:45-51
The mobile portal target (#mobile-header-search-portal) was removed, butHeaderSearchButtonstill attempts to render into it. Result: the mobile header search button will never appear via portal, and the new search icon button added here has noonClickhandler, so search is effectively broken on mobile. -
Maintainability |
lib/agents/tools/drawing.tsx:16-16
The drawing tool introduces multiple unsafe-but-type-valid patterns (any[],(params as any).location,units as any) and removes the earlier guarded parsing/timeout behavior for MCP calls. The current JSON parsing path (JSON.parse(...)) is unguarded; if the MCP returns non-JSON or unexpected content, the tool will throw and you’ll lose the opportunity to provide a controlled fallback. Also, returning{ type: 'DRAWING_TRIGGER', error }withouttimestampchanges the output shape consumers might rely on. -
Maintainability |
lib/utils/index.ts:121-124
getGoogleStaticMapUrlno longer throws whenGOOGLE_MAPS_API_KEYis missing, and will generate a URL containingkey=undefined. That creates confusing failures downstream (broken images) and makes debugging harder. The previous explicit error was better for correctness.
Summary of changes
Summary
This PR is a branch reset to bf6043b, which effectively reverts/discards later commits and restores earlier versions of multiple files.
Notable code-level deltas visible in the diff
- Chat + map context wiring:
components/chat.tsxnow readsdrawnFeatures/cameraStatefromuseMapData()and callsupdateDrawingContext(id, …)when they change; also changes the router refresh effect. - Header search button behavior:
components/header-search-button.tsxsimplifies portal discovery logic and switches notifications fromsonnertoreact-toastify; changes the disabled condition to depend onmap. - Map query handling types:
components/map/map-query-handler.tsxremoves the sharedToolOutputimport and inlines aToolOutput/McpResponseDatadefinition. - Mobile UI:
components/mobile-icons-bar.tsxremoves the mobile portal target and adds a standalone search icon button (without click behavior). - Drawing tool + schema:
lib/agents/tools/drawing.tsxloosens typing (any[],(params as any).location,units as any), simplifies MCP call parsing, and changes returned payload fields.lib/schema/drawing.tsxrelaxes validation (removes lat/lng bounds and the circle “location or center required” refinement; makes line coordinates optional).
- Utilities:
lib/utils/index.tschangesgetGoogleStaticMapUrlto a simple string template and no longer throws whenGOOGLE_MAPS_API_KEYis missing.lib/utils/mcp.tslogs partial secrets (first 8 chars) and removesthrowOnErrorfromcloseClient.
- Type file removed:
lib/types/tools.tsdeleted; equivalent types are partially duplicated elsewhere.
| import { nanoid } from 'nanoid' | ||
| import { UserMessage } from './user-message' | ||
| import { toast } from 'sonner' | ||
| import { toast } from 'react-toastify' | ||
| import { useSettingsStore } from '@/lib/store/settings' | ||
| import { useMapData } from './map/map-data-context' |
There was a problem hiding this comment.
Switching from sonner to react-toastify is a breaking change unless the app is already configured with ToastContainer and CSS imports. In this diff there’s no corresponding setup, so runtime toasts may silently fail or look unstyled.
Suggestion
Either revert to the previously configured toast library (sonner) or ensure react-toastify is globally configured (e.g., <ToastContainer /> in the root layout and the CSS import once). If you keep react-toastify, add/verify the app-wide setup in the same PR to avoid a partial migration.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| // Define the expected structure of the mcp_response from geospatialTool | ||
| interface McpResponseData { | ||
| location: { | ||
| latitude?: number; | ||
| longitude?: number; | ||
| place_name?: string; | ||
| address?: string; | ||
| }; | ||
| mapUrl?: string; | ||
| } | ||
|
|
||
| interface ToolOutput { | ||
| type: string; | ||
| originalUserInput?: string; | ||
| timestamp: string; | ||
| mcp_response?: McpResponseData | null; | ||
| features?: any[]; | ||
| error?: string | null; | ||
| } |
There was a problem hiding this comment.
ToolOutput is redefined locally and includes features?: any[], while the shared lib/types/tools.ts was deleted. This creates type drift risk across the codebase and makes it easy for tool producers/consumers to diverge silently.
Suggestion
Reintroduce a single shared ToolOutput type (e.g., lib/types/tools.ts) and import it in both tool producers and UI consumers. Avoid any[] for features—use geojson.Feature[] (or a narrower type used across drawing/map components).
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| lat: z.number(), | ||
| lng: z.number() | ||
| })).optional().describe('List of coordinates for the polygon vertices'), | ||
| label: z.string().optional().describe('Label for the polygon'), | ||
| color: z.string().optional().describe('Color for the polygon (e.g., "#ff0000")') | ||
| }), | ||
| z.object({ | ||
| type: z.literal('line'), | ||
| coordinates: z.array(coordinateSchema).min(2).describe('List of coordinates for the line segments'), | ||
| location: z.string().optional().describe('Name of the place to draw a line at'), | ||
| coordinates: z.array(z.object({ | ||
| lat: z.number(), | ||
| lng: z.number() | ||
| })).optional().describe('List of coordinates for the line segments'), | ||
| label: z.string().optional().describe('Label for the line'), | ||
| color: z.string().optional().describe('Color for the line (e.g., "#0000ff")') | ||
| }), | ||
| z.object({ | ||
| type: z.literal('circle'), | ||
| location: z.string().optional().describe('Name of the place to draw a circle around'), | ||
| center: coordinateSchema.optional().describe('Center coordinates for the circle'), | ||
| radius: z.number().positive().describe('Radius of the circle'), | ||
| center: z.object({ | ||
| lat: z.number(), | ||
| lng: z.number() | ||
| }).optional().describe('Center coordinates for the circle'), | ||
| radius: z.number().describe('Radius of the circle'), | ||
| units: z.enum(['meters', 'kilometers', 'miles', 'feet']).default('kilometers').describe('Units for the radius'), | ||
| label: z.string().optional().describe('Label for the circle'), | ||
| color: z.string().optional().describe('Color for the circle (e.g., "#00ff00")') | ||
| }) | ||
| ]).refine(data => { | ||
| if (data.type === 'circle') { | ||
| return Boolean(data.location || data.center); | ||
| } | ||
| return true; | ||
| }, { | ||
| message: 'Either location or center must be provided for circles' | ||
| }); | ||
| ]); |
There was a problem hiding this comment.
drawingToolSchema was weakened significantly:
- removed lat/lng bounds
- made
line.coordinatesoptional (but the executor throws if missing) - removed
circlerefinement enforcinglocation || center - removed
radius.positive()constraint
This increases invalid tool calls and pushes validation failures into runtime errors, which is worse for reliability and model/tool behavior.
Suggestion
Restore strict validation:
- bring back coordinate bounds
- keep
line.coordinatesrequired with.min(2) - keep
circle.radiusas.positive() - re-add
.refineto enforcelocation || centerfor circles (and possibly polygons)
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| console.log('[MCP Utility] Environment check:', { | ||
| composioApiKey: composioApiKey ? 'PRESENT' : 'MISSING', | ||
| mapboxAccessToken: mapboxAccessToken ? 'PRESENT' : 'MISSING', | ||
| composioUserId: composioUserId ? 'PRESENT' : 'MISSING', | ||
| composioApiKey: composioApiKey ? `${composioApiKey.substring(0, 8)}...` : 'MISSING', | ||
| mapboxAccessToken: mapboxAccessToken ? `${mapboxAccessToken.substring(0, 8)}...` : 'MISSING', | ||
| composioUserId: composioUserId ? `${composioUserId.substring(0, 8)}...` : 'MISSING', | ||
| }); |
There was a problem hiding this comment.
This now logs the first 8 characters of secrets. That’s still sensitive information and can materially help an attacker correlate/validate leaked keys; it also violates the common best practice of never logging secrets (even partially). Earlier code only logged PRESENT/MISSING, which is safer.
Suggestion
Revert to non-secret logging (PRESENT/MISSING) or log only boolean flags. If you need debugging, gate more detailed logging behind a local-only dev flag and ensure it cannot run in production.
Example:
console.log('[MCP Utility] Environment check:', {
composioApiKey: composioApiKey ? 'PRESENT' : 'MISSING',
mapboxAccessToken: mapboxAccessToken ? 'PRESENT' : 'MISSING',
composioUserId: composioUserId ? 'PRESENT' : 'MISSING',
})Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
ngoiyaeric
merged commit b6a1992
into
feature/radial-drawing-tool-15881425069546006738
2 participants
User description
The user requested to make commit bf6043b the head of the branch feature/radial-drawing-tool-15881425069546006738 and discard later changes. This was achieved via git reset --hard bf6043b.
PR created automatically by Jules for task 5794459265562258649 started by @ngoiyaeric
PR Type
Bug fix, Enhancement
Description
Removed deleted type definitions and simplified utility functions
Refactored drawing tool to remove type safety constraints and simplify logic
Improved environment variable logging with partial masking for security
Simplified map state management and portal detection in header search
Updated toast library from sonner to react-toastify
Cleaned up schema validation and removed unnecessary type constraints
Diagram Walkthrough
File Walkthrough
1 files
Removed tool type definitions file7 files
Simplified Google Maps URL generationImproved logging and simplified client closureSimplified portal detection and map availability checksInlined type definitions and simplified error handlingReplaced portal with direct search buttonRemoved type imports and simplified feature handlingInlined coordinate schema and removed validation rules1 files
Simplified router refresh logic and state management1 files
Updated CSS classes for glassmorphic styling1 files
Reordered tool decision flow and removed priority note