Improve visual editing tools#469
Conversation
|
Hi @gustavohenrip! 🎉 Thanks for making open-design better! |
lefarcen
left a comment
There was a problem hiding this comment.
Hi @gustavohenrip! 👋
Thanks for this comprehensive PR. I've reviewed the visual editing tools, conversation compaction, and related changes. The features look very promising — functional edit/draw/tweak flows will significantly improve the design workflow. Here are my findings:
Code correctness (Lens A)
✅ Security: Good CSS sanitization — safeTweakValue properly blocks javascript:, expression(), @import, and dangerous characters. The inline style application through setInlineStyle is safe.
✅ Visual edit preview — The iframe message-based preview system correctly isolates edit drafts from the actual DOM until save.
✅ Conversation compaction — The logic preserves recent messages intact and only summarizes older content. The 32K trigger threshold and 8-message recent window are reasonable.
/api/active, /api/mcp/install-info, /api/projects/:id/events, /api/projects/:id/search, /api/projects/:id/archive/batch. While the PR note says "Build and tests were not run locally," these deletions could break existing features. Recommendation: verify each removed route isn't used by the web UI or external integrations. If they're truly dead code, document why in the PR description.
FileViewer.tsx holds large state objects (editBatch, liveCommentTargets, tweakTokens). For PRs with 100+ elements, this could cause performance issues. Consider adding cleanup on unmount and state size limits.
Reasoning completeness (Lens B)
content, attachments, events, etc. What happens if a message is malformed or has unexpected structure? Should there be error boundaries? The current code would silently skip parts or crash. Recommendation: add defensive checks or wrap in try/catch.
subscribeFileEvents. The PR removes this entirely. Does the new flow rely on manual refresh? Or is live-reload now handled differently? The PR description doesn't explain this trade-off. If live-reload is intentionally dropped, that should be called out as a UX change.
extractTweakTokens caps at 64 tokens and uses a simple regex. What happens with CSS variables defined in @media or nested selectors? They're skipped. For design systems with extensive theming, 64 might not be enough. Consider documenting this limitation or making it configurable.
corepack enable or a clearer error message.
Summary
This PR adds valuable features and the core implementation is solid. The main concerns are:
- Verify removed server routes — ensure nothing breaks in production
- Compaction error handling — add defensive checks for malformed messages
- Document live-reload trade-off — clarify if file events are intentionally removed
Overall: Good work on the visual editing flows. The CSS sanitization is thorough, and the preview-before-save design is the right UX. Once the above points are clarified, this will be ready to ship.
— open-design team
Review follow-upThis update addresses the review feedback around API compatibility, compaction safety, memory management, and launcher portability. Server routes and runtime compatibility
Conversation compaction hardening
FileViewer memory/state safety
Tweaks limitation clarification
Launcher portability
|
Summary
This update expands Open Design's visual editing workflow and agent behavior so the app can work much closer to the Claude Design experience, with stronger direct manipulation tools, better HTML editing safety, clearer runtime feedback, and a more complete prompt/harness setup.
Claude Design prompt/harness alignment
Edit mode
Draw mode
Tweaks mode
Preview and element-targeting improvements
Artifact and file-update safety
Conversation compaction and status feedback
Tooling and launch workflow
.commandlauncher so the project can be started more easily from Finder.User impact