-
Notifications
You must be signed in to change notification settings - Fork 689
feat: fix backward compatibility pg 14 for postgres trigger #5851
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: fix backward compatibility pg 14 for postgres trigger #5851
Conversation
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.
Caution
Changes requested ❌
Reviewed everything up to da0a50a in 2 minutes and 0 seconds. Click for details.
- Reviewed
2658
lines of code in27
files - Skipped
0
files when reviewing. - Skipped posting
12
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-api/src/triggers.rs:35
- Draft comment:
The query for primary_schedule uses the same parameter ($1) for both 'path' and 'script_path'. Verify if this is intended, as it forces these two values to be identical. If they should be separate, adjust the query parameters accordingly. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. frontend/src/lib/components/triggers/CaptureWrapper.svelte:188
- Draft comment:
The reactive effect$effect(() => { args && (captureActive = false) })
resets the captureActive flag whenever 'args' is truthy. Ensure that this behavior is intended—as it may inadvertently disable capture when arguments are provided. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. frontend/src/lib/components/triggers/postgres/RelationPicker.svelte:23
- Draft comment:
The reactive block mutates the 'relations' prop directly when pg14 is true. Mutating props can lead to unexpected side effects. Consider cloning or deriving a local copy before making modifications. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% In Svelte, mutating props directly is actually a valid pattern, unlike React. Svelte's reactivity system handles prop mutations gracefully. The code is using Svelte's reactive statements ($:) which is the idiomatic way to handle prop changes. Creating defensive copies would add complexity without clear benefits in this case. The comment comes from a React-centric view where prop mutation is an anti-pattern. But this is Svelte code where different patterns apply. While prop mutation would be problematic in React, Svelte's reactivity system is designed to handle prop mutations properly. The current implementation is idiomatic Svelte code. The comment should be deleted as it suggests applying React patterns to Svelte code where they don't apply. The current implementation is correct for Svelte.
4. frontend/src/lib/components/triggers/postgres/utils.ts:151
- Draft comment:
The 'getDefaultTableToTrack' function returns a default object based on the 'pg14' flag. Consider adding inline documentation explaining why for pg14 an array with a default empty table is returned versus an empty array otherwise. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
5. backend/.vscode/settings.json:13
- Draft comment:
Typo detected: "restructring" appears to be a misspelling of "restructuring". Please update the text to correct the typo. - Reason this comment was not posted:
Comment was on unchanged code.
6. backend/windmill-api/src/postgres_triggers/handler.rs:837
- Draft comment:
Typographical error in the comment: consider rephrasing "if all table is none it means that the publication do not exist in the database" to "if all_table is None, it means that the publication does not exist in the database". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment correctly identifies a grammatical error, comments about purely cosmetic issues like grammar in code comments are not substantive enough to warrant a PR comment. The meaning is still clear despite the grammatical errors. This doesn't affect functionality or code quality in any meaningful way. The grammar errors could make the code slightly harder to understand for non-native English speakers. Clear documentation is important for maintainability. While clear documentation is valuable, this particular grammar issue doesn't significantly impact comprehension. The meaning remains clear enough, and this kind of minor polish can be handled during regular code maintenance rather than PR review. Delete this comment as it addresses a minor grammatical issue that doesn't materially impact code quality or functionality.
7. backend/windmill-api/src/postgres_triggers/handler.rs:869
- Draft comment:
Typo: It looks like the assignment tofirst
is missing a semicolon. Please add a semicolon at the end of line 869. - Reason this comment was not posted:
Comment looked like it was already resolved.
8. backend/windmill-api/src/postgres_triggers/handler.rs:1089
- Draft comment:
Typo: The variable 'occuped' appears to be misspelled. Please update it to 'occupied' for clarity. - Reason this comment was not posted:
Comment was on unchanged code.
9. backend/windmill-api/src/postgres_triggers/handler.rs:1180
- Draft comment:
Typo in debug message: "does not exists" should be "does not exist". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct about the grammar, debug messages are internal logging statements not exposed to users. The rules state not to make comments that are obvious or unimportant. A minor grammatical fix in a debug message seems too trivial to warrant a comment. The grammar error could indicate a lack of attention to detail that might manifest in more serious ways elsewhere in the code. Debug messages are still read by developers. While code quality is important, a minor grammatical error in an internal debug message has no functional impact and fixing it provides minimal value. The rules specifically say not to make unimportant comments. This comment should be deleted as it addresses a trivial grammatical issue in a debug message, which falls under the category of unimportant changes per the review rules.
10. frontend/src/lib/components/triggers/gcp/GcpTriggerEditorConfigSection.svelte:71
- Draft comment:
Typo found: The function name 'listAllTgoogleTopicSubscriptions' appears to include an extra 'T' at the beginning of 'Google'. Please verify if the correct function name should be 'listAllGoogleTopicSubscriptions'. - Reason this comment was not posted:
Comment was on unchanged code.
11. frontend/src/lib/components/triggers/postgres/PostgresTriggerEditorInner.svelte:90
- Draft comment:
Typo: Consider using 'TypeScript' instead of 'Typescript' for the language name to match conventional capitalization. - Reason this comment was not posted:
Comment was on unchanged code.
12. frontend/src/lib/components/triggers/postgres/PostgresTriggerEditorInner.svelte:785
- Draft comment:
There's a potential typographical issue with the closing tag on this line. The string "</div" seems to be missing a closing angle bracket. Please consider merging this line with the following line (or modifying the lines) to properly form a complete closing tag, for example: "". - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_lEwzY0sUKkaEBrdX
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
query.push_str(" TABLE ONLY "); | ||
if pg_14 && first { | ||
query.push_str(" TABLE ONLY "); | ||
first = false |
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.
Typographical error: Missing semicolon at the end of the line first = false
. Please add a semicolon.
first = false | |
first = false; |
Deploying windmill with
|
Latest commit: |
75c6b37
|
Status: | ✅ Deploy successful! |
Preview URL: | https://1ad0e8d2.windmill.pages.dev |
Branch Preview URL: | https://dieri-fix-backward-compatibi-z1gr.windmill.pages.dev |
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.
Important
Looks good to me! 👍
Reviewed 75c6b37 in 34 seconds. Click for details.
- Reviewed
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. backend/windmill-api/src/postgres_triggers/mod.rs:180
- Draft comment:
Added missing semicolon for proper statement termination. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_jNWOaQI3JWIDJ8xF
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Important
Fixes PostgreSQL 14 compatibility for triggers by updating backend logic and frontend UI to handle limitations and provide better error handling.
get_postgres_version_internal()
andget_postgres_version()
inhandler.rs
to fetch PostgreSQL version.create_pg_publication()
andcreate_logical_replication_slot()
inmod.rs
to handle PostgreSQL 14 limitations.set_postgres_trigger_config()
incapture.rs
to support basic and advanced modes.mod.rs
.PostgresTriggerEditorInner.svelte
to handle PostgreSQL 14 limitations in UI.RelationPicker.svelte
to disable column-specific tracking for PostgreSQL 14.PublicationPicker.svelte
andSlotPicker.svelte
for better user feedback and loading states.GcpTriggerEditorConfigSection.svelte
with loading indicators and error handling.This description was created by
for 75c6b37. You can customize this summary. It will automatically update as commits are pushed.