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

Sort activated feature gates #473

Merged
merged 1 commit into from
Feb 27, 2025
Merged

Sort activated feature gates #473

merged 1 commit into from
Feb 27, 2025

Conversation

ngundotra
Copy link
Collaborator

@ngundotra ngundotra commented Feb 27, 2025

Setup feature gate sorting so that activated gates (mainnet, devnet) show up last, but their intra-ordering preserved

Screenshot 2025-02-26 at 11 57 44 PM

Copy link

vercel bot commented Feb 27, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
explorer ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 27, 2025 4:58am

Copy link

@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 2f5f5ef in 2 minutes and 7 seconds

More details
  • Looked at 63 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 drafted comments based on config settings.
1. app/utils/feature-gate/featureGate.tsx:37
  • Draft comment:
    The isActivated function uses different properties for filtering (e.g., devnetActivationEpoch for MainnetBeta) versus sorting (mainnetActivationEpoch). Confirm this divergence is intentional.
  • 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%
    The different criteria serve different purposes - filtering shows upcoming features (activated in lower envs) while sorting separates currently active vs inactive features. This is a reasonable and intentional design. The comment is asking for confirmation rather than pointing out an actual issue. This violates our rule about not asking authors to confirm their intentions.
    Maybe there could be a genuine bug here that I'm missing? The logic is complex enough that it's worth a second look.
    After reviewing the logic again, the different criteria make perfect sense for their respective purposes. The filtering logic determines which features to show, while the sorting logic determines their display order based on activation status.
    This comment should be deleted as it merely asks for confirmation of an intentional design choice rather than identifying an actual issue.
2. app/utils/feature-gate/featureGate.tsx:50
  • Draft comment:
    Sorting assumes a stable sort to preserve existing ordering. Verify that the JS environment guarantees stability or consider using a stable sort implementation.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 30% <= threshold 50%
    The comment is asking the author to verify the stability of the sort in the JS environment. This is a request for confirmation, which violates the rule against asking the author to confirm or verify things. However, it does suggest considering a stable sort implementation, which is a specific code suggestion. The comment is partially useful but mostly violates the rules.
3. app/utils/feature-gate/featureGate.tsx:36
  • Draft comment:
    Consider refactoring the isActivated helper function outside the sort callback for improved readability and performance.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
4. app/utils/feature-gate/featureGate.tsx:35
  • Draft comment:
    Mismatch between filter and sort activation checks. For MainnetBeta and Devnet, filtering uses devnet/testnet activation epochs while sorting uses mainnet/devnet. If intentional, please document the rationale.
  • 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%
    Looking at the code more carefully, the filtering and sorting serve different purposes. Filtering determines which features to show based on their progression through test environments. Sorting just determines display order based on whether a feature is active in the current environment. These are logically distinct concerns and the different epoch checks make sense for their respective purposes.
    I could be wrong about the intended feature progression logic. Maybe there's a subtle bug here that could cause features to be displayed incorrectly.
    The code comments clearly explain the filtering logic's purpose, and the sorting logic is separately documented as just affecting display order. The different epoch checks serve distinct, valid purposes.
    The comment should be deleted as it flags a "mismatch" that is actually intentional and correct - the filtering and sorting logics serve different purposes and appropriately use different activation checks.
5. app/utils/feature-gate/featureGate.tsx:35
  • Draft comment:
    For Cluster.Testnet, the filter forces testnetActivationEpoch === null, so the sort comparator always returns 0. Confirm if sorting is needed or add a comment 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 = 10% vs. threshold = 50%
    The filter ensures testnetActivationEpoch is null for Testnet cluster. The sort function then checks if testnetActivationEpoch is not null, which will always be false for Testnet due to the filter. This means for Testnet, all features will have isActivated=false, making the sort effectively a no-op. However, the sort function is still logically correct and maintains consistency across all cluster types.
    The sort function being a no-op for Testnet isn't necessarily a problem - it's just an implementation detail. The code is still correct and maintainable.
    The comment suggests a potential issue but doesn't identify any actual problem. The code works as intended even if some logic paths are redundant.
    This comment should be deleted as it doesn't identify a real issue requiring changes. The code is correct even if the sort becomes a no-op for Testnet.
6. app/utils/feature-gate/featureGate.tsx:35
  • Draft comment:
    Consider extracting the isActivated function outside the sort comparator to simplify the code and potentially improve readability.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None

Workflow ID: wflow_GR17fYzju9FyCr1o


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

@ngundotra ngundotra merged commit 8c16a7e into master Feb 27, 2025
3 checks passed
@ngundotra ngundotra deleted the feature-gate-sorting branch February 27, 2025 05:00
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.

1 participant