-
Notifications
You must be signed in to change notification settings - Fork 574
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
feat: captures #4807
feat: captures #4807
Conversation
Deploying windmill with Cloudflare Pages
|
…ggerspreprocessor-capture
f85cf46
to
2c5d07f
Compare
4ea13d3
to
95c613d
Compare
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.
❌ Changes requested. Reviewed everything up to ffdab59 in 2 minutes and 54 seconds
More details
- Looked at
9524
lines of code in102
files - Skipped
2
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. backend/windmill-api/src/capture.rs:12
- Draft comment:
Verify that increasingKEEP_LAST
from 8 to 20 aligns with system capacity and intended behavior. - Reason this comment was not posted:
Confidence changes required:50%
TheKEEP_LAST
constant was changed from 8 to 20, which affects the number of recent captures retained. This change should be verified to ensure it aligns with the intended behavior and system capacity.
2. frontend/src/lib/components/details/CopyableCodeBlock.svelte:1
-
Draft comment:
This functionality already exists inHighlightCode
. Consider using or extending that component instead. -
component
HighlightCode
(HighlightCode.svelte) -
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
While there is overlap in functionality, CopyableCodeBlock appears to serve a different specific use case with its disabled state and simpler interface. The differences seem intentional rather than accidental duplication. Forcing reuse here could make the code more complex rather than better.
The comment raises a valid point about code reuse and DRY principles. Having two components that both wrap svelte-highlight with copy functionality could lead to maintenance issues.
Sometimes having two simpler, focused components is better than one complex component. The differences here seem intentional and trying to force reuse could add unnecessary complexity.
While the comment raises a valid point about potential code duplication, the differences between the components appear intentional and valuable. The comment should be removed as it wouldn't lead to better code.
Workflow ID: wflow_2djoEnFKpWYgnD1J
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@@ -0,0 +1,55 @@ | |||
<script lang="ts"> |
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.
This is a duplicate of the existing Section component. The components are functionally identical with only cosmetic differences. Consider using Section with small=true instead of creating a new component.
- component
Section
(Section.svelte)
Important
Add capture handling feature with backend SQL and API updates, and frontend UI enhancements.
.sqlx
files.openapi.yaml
to handle capture configurations.capture.rs
for setting and pinging capture configurations.FlowEditorPanel.svelte
andScriptEditor.svelte
to handle capture-related actions.CaptureButton.svelte
andCaptureTable.svelte
for UI interactions.svelte.config.js
to includepreprocessMeltUI
for UI enhancements.+page.svelte
to include capture-related states.This description was created by for ffdab59. It will automatically update as commits are pushed.