-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Improve agent viewport zoom behavior #6882
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
Deploying with
|
Status | Name | Latest Commit | Updated (UTC) |
---|---|---|---|
❌ Deployment failed View logs |
agent-template | d606be8 | Oct 06 2025, 04:10 PM |
🤖 Claude Code Review SummaryThis PR improves the agent viewport zoom behavior by mapping the user's zoom level to a constrained range (75%-125%) instead of tracking it 1:1. This is a sensible UX improvement that should help the agent operate more effectively at various zoom levels. Code Quality ✅Good practices observed:
Issues found: 1. Typo in method name (line 723)calculatViewportBounds() // Missing 'e' in 'calculate' Should be: This typo appears in:
2. Box mutation concern (line 742-753)The function scaleBoxFromCenter(box: Box, scale: number) {
// ... mutates box properties directly
box.x = center.x - newWidth / 2
box.y = center.y - newHeight / 2
box.w = newWidth
box.h = newHeight
return box
} This is potentially problematic because:
Recommendation: Create a new Box instead: function scaleBoxFromCenter(box: Box, scale: number) {
const { center } = box
const newWidth = box.w * scale
const newHeight = box.h * scale
return new Box(
center.x - newWidth / 2,
center.y - newHeight / 2,
newWidth,
newHeight
)
} Logic & Correctness
|
🤖 Claude Code Review SummaryThis PR improves the agent's viewport zoom behavior by mapping the user's zoom level to a constrained range (75%-125%) instead of tracking it 1:1. The implementation is clean and well-documented. Code Quality ✅Strengths:
Minor Style Observations:
Implementation Review ✅The zoom mapping logic is sound: const mappedZoom = modulate(editor.getZoomLevel(), [zoomMin, zoomMax], [1.25, 0.75]) This maps the user's zoom from their full available range to [1.25, 0.75], meaning:
This inverse relationship makes sense: when the user zooms in to see details, the agent should have a tighter viewport to focus on relevant content. Potential Considerations1. Edge Cases
|
🤖 Claude Code Review SummaryThis PR appears to be a massive initial import of the entire tldraw codebase (~5000+ files). Given the scope, this review focuses on high-level architectural observations and key areas to verify. Repository Structure ✅The monorepo structure follows solid engineering practices:
Key ObservationsStrengths 👍
Recommendations 🔍1. Review Secret ManagementSince this is a full repository import, verify that no secrets were accidentally committed:
2. Dependency AuditWith this many packages, recommend running:
To identify any known vulnerabilities in dependencies. 3. License ComplianceVerify all third-party dependencies are compatible with your MIT license:
4. Build System VerificationTest the complete build pipeline:
5. Code Quality Checks
Architecture Highlights 🏗️Reactive State Management
Multiplayer Architecture
Testing Strategy
Potential Issues
|
🤖 Claude Code Review SummaryThis PR improves the agent viewport zoom behavior by decoupling the agent's viewport zoom from the user's zoom level. The agent's zoom is now clamped between 75% and 125% (0.67-1.25 in code) to provide a more consistent working area for AI agents. Code Quality ✅Strengths:
Issues Found: 1. 🔴 Debug Console.log Left in Production CodeLocation: console.log('messages', JSON.stringify(messages, null, 2)) This debug statement should be removed before merging to production. It could:
Recommendation: Remove this line or use a proper logging framework with configurable log levels. 2. 🟡 Typo Fix in buildSystemPrompt.tsLocation: Good catch fixing the typo ( 3. 🟡 Magic Numbers in Zoom ClampingLocation: const clampedZoom = clamp(editor.getZoomLevel(), 0.67, 1.25) The values
Recommendation: const MIN_AGENT_ZOOM = 0.67 // Equivalent to 150% user zoom (1/1.5)
const MAX_AGENT_ZOOM = 1.25 // Equivalent to 80% user zoom (1/0.8)
const clampedZoom = clamp(editor.getZoomLevel(), MIN_AGENT_ZOOM, MAX_AGENT_ZOOM) Or add a comment explaining the inverse relationship if that's the intent. Architecture & Design ✅Positive:
Suggestion: Performance Considerations ✅
Testing 🟡Missing:
Recommendation:
Security ✅No security concerns identified. The changes are isolated to viewport calculations and don't involve user input validation or data persistence. Documentation 📝Good:
Could Improve:
Summary of RecommendationsMust Fix:
Should Consider:
Nice to Have:
Overall AssessmentThis is a well-implemented improvement that addresses a real UX issue with agent viewport behavior. The code is clean and maintainable, with only minor issues to address before merging. The debug console.log is the only blocking issue. Approval Status: Approve pending removal of console.log statement. |
@@ -153,7 +153,7 @@ Refer to the JSON schema for the full list of available events, their properties | |||
- You will be provided with list of shapes that are outside of your viewport. | |||
- You can use the \`setMyView\` action to change your viewport to navigate to other areas of the canvas if needed. This will provide you with an updated view of the canvas. You can also use this to functionally zoom in or out. | |||
- Never send any events after you have used the \`setMyView\` action. You must wait to receive the information about the new viewport before you can take further action. | |||
- Always make sure that any shapes you create or modify are within your viewport. | |||
- Always make sure that any shapes you create or modify are within your viewport. Move your viewport to ensure that the shapes are within your view. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this PR still being worked on? We should remove this first sentence — We don't want it to stay inside its viewport.
fixed in a separate PR |
Changes the agent's viewport zoom to no longer track the user's zoom 1:1.
The agent's viewport zoom now stays between 75% and 125%, depending on the user's zoom level
Change type
bugfix
improvement
feature
api
other