Skip to content
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: Add an array option type #3121

Open
wants to merge 55 commits into
base: dev
Choose a base branch
from

Conversation

EepyElvyra
Copy link
Contributor

@EepyElvyra EepyElvyra commented Jan 3, 2025

Adds an array option type, this replaces the previously used strings with separators.
IDs are not very user-friendly, therefore this PR also implements a Modal which replicates the functionality of Discord's forwarding Modal to search for users/channels/guilds by text.

implements: #615 #2210

Settings sync is fully functional with these changes

Filtering for names/roles when it comes to users is difficult to implement and out of scope for this PR, it also isn't needed in any existing plugin. Therefore it will be omitted. Searching for user names remains possible through the search modal.

Repository owner deleted a comment Jan 3, 2025
@EepyElvyra
Copy link
Contributor Author

I think this is ready now

@EepyElvyra EepyElvyra marked this pull request as ready for review January 11, 2025 01:21
@EepyElvyra
Copy link
Contributor Author

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

CodiumAI-Agent commented Jan 11, 2025

PR Reviewer Guide 🔍

(Review updated until commit 14fe735)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

615 - Fully compliant

Fully compliant requirements:

  • Replace the use of strings with separators for settings with an array option type.
  • Implement a user-friendly modal for managing array inputs.
  • Ensure settings sync functionality is preserved.

Not compliant requirements:

  • None

2210 - Partially compliant

Fully compliant requirements:

  • Provide an API for allow/deny lists for users, guilds, and channels.
  • Implement a UI helper for managing these lists.

Not compliant requirements:

  • Allow filtering by ID, name substring, and guild role for user lists.
⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

The SettingArrayComponent does not handle cases where pluginSettings[id] is undefined, which could lead to runtime errors.

if (items.length === 0 && pluginSettings[id].length !== 0) {
    setItems(pluginSettings[id]);
}
Data Migration

Ensure the migration logic for tags correctly handles edge cases, such as duplicate or invalid data, to prevent data loss or corruption.

if (!settings.store.migrated) {
    const data = await DataStore.get(DATA_KEY);
    if (data !== undefined) settings.store.data = data;
    settings.store.migrated = true;
}

@EepyElvyra
Copy link
Contributor Author

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

Persistent review updated to latest commit 14fe735

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants