Some tokens are falsely labeled as broken

[robinhoodie0823]

20231027_151149.mp4

Closes #2301

[changeset-bot]

🦋 Changeset detected

Latest commit: 77769d6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@tokens-studio/figma-plugin	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

PR Analysis

• 🎯 Main theme: Fixing a bug where some tokens are falsely labeled as broken.
• 📝 PR summary: This PR aims to fix a bug where some tokens are falsely labeled as broken. It includes changes in three files: 'Tokens.tsx', 'TokenResolver.ts', and 'tokenHelpers.ts'. The changes mainly involve refactoring and bug fixing.
• 📌 Type of PR: Bug fix
• 🧪 Relevant tests added: No
• ⏱️ Estimated effort to review [1-5]: 3, because the PR involves changes in multiple files and the logic seems to be complex, but the changes themselves are not too extensive.
• 🔒 Security concerns: No security concerns found

PR Feedback

• 💡 General suggestions: It would be beneficial to add some unit tests to verify the correctness of the changes. Also, it would be helpful to add some comments explaining the logic behind the changes, especially in the 'TokenResolver.ts' file.

• 🤖 Code feedback:

  • relevant file: src/utils/TokenResolver.ts
    suggestion: It seems like the line checking for failing references has been commented out and the 'failedToResolve' property has been removed from the 'resolvedToken'. If this is intentional, please make sure that this does not introduce any unintended side effects. [important]
    relevant line: ...token, value: finalValue, rawValue: token.value, ...{},

  • relevant file: src/app/components/Tokens.tsx
    suggestion: The 'tokenType' variable is declared but never used. If it's not needed, consider removing it to clean up the code. [medium]
    relevant line: const tokenType = useSelector(tokenTypeSelector);

How to use

To invoke the PR-Agent, add a comment using one of the following commands:
/review [-i]: Request a review of your Pull Request. For an incremental review, which only considers changes since the last review, include the '-i' option.
/describe: Modify the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest improvements to the code in the PR. Extended mode employs several calls, and provides a more thorough feedback.
/ask <QUESTION>: Pose a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.

To edit any configuration parameter from configuration.toml, add --config_path=new_value
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, use the /config command.

[SorsOps]

@robinhoodie0823 Can you provide the testing for this PR inside for a valid case where the resolution algorithm is showing a falsely labelled broken token ? This just looks like a PR regressing the functionality to ever show an example of a broken reference

[six7]

Tests are failing here

[six7]

@robinhoodie0823 any updates on this one?

[robinhoodie0823]

No, I'm working on other tickets now. I will fix asap.

[github-actions]

Commit SHA:e4a29779859132dcdfc9ae5340a5a184f239a26a

Test coverage results 🧪

Code coverage diff between base branch:main and head branch: 2301-some-tokens-are-falsely-labeled-as-broken 

Status	File	% Stmts	% Branch	% Funcs	% Lines
🟢	total	70.81 (0.01)	61.13 (0.02)	68.88 (0)	71.06 (0)
🟢	src/utils/TokenResolver.ts	94.87 (0.14)	82.29 (0.77)	100 (0)	94.82 (0.13)

[github-actions]

Commit SHA:0b1839b1e2e75f04571a3994c3148741e3e4eeda
Current PR reduces the test coverage percentage by 100 for some tests

[six7]

As @SorsOps already stated we need a test for this scenario.

Also, it seems you only removed the check if something is failing. What needs to be fixed is the fact that some tokens are falsely labeled as broken, not that we dont want tokens to show up as broken anymore.

The problem according to the issue is about typography, border, shadow tokens.

[six7]

Why remove this? This token should be classified as failedToResolve because it did fail to resolve. We cant just remove this.

[six7]

I think this won't work for tokens that have another object inside, such as composite tokens or shadow tokens. Is there another way you can stringify the object that does not produce curly brackets?

For example right now the transformer tests are failing because of this

[six7]

Great. Can you add a test to make sure we have this behavior covered going forward? (We should write/update tests for any PR we make)

[six7]

✨