Skip to content

fix(frontend): postgres remove selectedTable #5386

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

Merged
merged 11 commits into from
Apr 16, 2025

Conversation

Guilhem-lm
Copy link
Contributor

@Guilhem-lm Guilhem-lm commented Feb 26, 2025

Important

Refines frontend PostgreSQL trigger components by removing selectedTable, adjusting relation handling, and minor backend import updates.

  • Frontend:
    • In PostgresEditorConfigSection.svelte, removed console.log from updateValidity().
    • In PostgresTriggerEditorInner.svelte, changed relations type to Relations[] | undefined and updated getTemplateScript() to handle undefined relations.
    • In RelationPicker.svelte, initialized selected based on relations length and updated bind:selected logic.
  • Backend:
    • In handler.rs, added drop_logical_replication_slot_query to imports.

This description was created by Ellipsis for 29c6437. It will automatically update as commits are pushed.

Copy link

cloudflare-workers-and-pages bot commented Feb 26, 2025

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: 29c6437
Status: ✅  Deploy successful!
Preview URL: https://511694f1.windmill.pages.dev
Branch Preview URL: https://fix-frontend-postgres-select.windmill.pages.dev

View logs

@rubenfiszel rubenfiszel force-pushed the main branch 2 times, most recently from 17e7f15 to a695621 Compare March 12, 2025 11:34
@rubenfiszel rubenfiszel force-pushed the main branch 2 times, most recently from edcef56 to 5573d88 Compare March 25, 2025 01:00
@dieriba dieriba force-pushed the fix/frontend/postgres_selected_tables branch 2 times, most recently from 18e7fca to 25b39e6 Compare April 8, 2025 21:59
Copy link

Meticulous was unable to execute a test run for this PR because the most recent commit is associated with multiple PRs. To execute a test run, please try pushing up a new commit that is only associated with this PR.

Last updated for commit 8219454. This comment will update as new commits are pushed.

@dieriba dieriba marked this pull request as ready for review April 16, 2025 16:22
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to 29c6437 in 3 minutes and 43 seconds

More details
  • Looked at 97 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 12 drafted comments based on config settings.
1. frontend/src/lib/components/triggers/postgres/PostgresEditorConfigSection.svelte:37
  • Draft comment:
    Removed debug console.log in updateValidity. Good cleanup – ensure no leftover debugging statements.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
2. frontend/src/lib/components/triggers/postgres/PostgresTriggerEditorInner.svelte:361
  • Draft comment:
    Replaced undefined check with emptyStringTrimmed for script_path. This improves consistency in empty string handling.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
3. frontend/src/lib/components/triggers/postgres/RelationPicker.svelte:16
  • Draft comment:
    Initialization of 'selected' based on existing relations is concise; ensure this logic aligns with UX expectations when relations are later modified.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
4. frontend/src/lib/components/triggers/postgres/PostgresEditorConfigSection.svelte:37
  • Draft comment:
    The validity check uses '!publication.table_to_track || publication.table_to_track.length === 0' to decide if the config is valid. Consider adding a comment stating that 'undefined' represents selection of all tables, to improve readability.
  • Reason this comment was not posted:
    Comment was on unchanged code.
5. frontend/src/lib/components/triggers/postgres/PostgresTriggerEditorInner.svelte:48
  • Draft comment:
    The variable 'relations' is typed as 'Relations[] | undefined' but initialized to an empty array. Consider initializing to undefined if the 'all tables' case is meant to be represented by undefined for clarity.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    The type suggests undefined is a valid state, and the code handles it. However, looking at the full file, relations is initialized to [] in openNew() and loadTrigger(), suggesting empty array is the intended default state. The undefined case seems to only be used for error checking. The comment may be overthinking this - the initialization seems fine given the actual usage patterns.
    I may be missing some subtle cases where undefined vs empty array semantics matter. The type declaration suggests undefined was intended to mean something specific.
    While undefined is allowed by the type, the code consistently treats empty array as the default state. The undefined check in getTemplateScript appears to be defensive programming rather than meaningful semantics.
    The comment raises a theoretical concern but the code's actual usage patterns show the current initialization is appropriate. This appears to be overthinking the implementation.
6. frontend/src/lib/components/triggers/postgres/PostgresTriggerEditorInner.svelte:305
  • Draft comment:
    The disabled property on the Save button has a long compound condition. Consider extracting it into a well-named computed property for improved readability and maintainability.
  • 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.
7. frontend/src/lib/components/triggers/postgres/RelationPicker.svelte:20
  • Draft comment:
    The check if (!relations || !Array.isArray(relations)) seems redundant given the type. If 'relations' is only undefined or an array, consider checking just for undefined to simplify the logic.
  • 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.
8. frontend/src/lib/components/triggers/postgres/RelationPicker.svelte:39
  • Draft comment:
    Using the 'cached' variable to restore 'relations' when toggling between 'all' and 'specific' is clever. Consider adding a brief comment to explain this mechanism for future maintainers.
  • 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.
9. backend/windmill-api/src/postgres_triggers/handler.rs:511
  • Draft comment:
    Typo: 'commiting' should be corrected to 'committing' for clarity.
  • 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.
10. backend/windmill-api/src/postgres_triggers/handler.rs:1284
  • Draft comment:
    Typo: 'succesfully' should be corrected to 'successfully'.
  • 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.
11. frontend/src/lib/components/triggers/postgres/PostgresTriggerEditorInner.svelte:23
  • Draft comment:
    Typo in the import path: '$lib/components/random_positive_adjetive' should be '$lib/components/random_positive_adjective'.
  • 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.
12. frontend/src/lib/components/triggers/postgres/PostgresTriggerEditorInner.svelte:120
  • Draft comment:
    Typographical error in the function parameter: 'nis_flow' on line 120 likely should be 'is_flow' for consistency with the rest of the file.
  • 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.

Workflow ID: wflow_qvNxvpiWHaO7jkHX


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@rubenfiszel rubenfiszel merged commit bd7c6a2 into main Apr 16, 2025
5 checks passed
@rubenfiszel rubenfiszel deleted the fix/frontend/postgres_selected_tables branch April 16, 2025 22:21
@github-actions github-actions bot locked and limited conversation to collaborators Apr 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants