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.

Sign up for GitHub

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

Jump to bottom

Unusual remapping renaming behaviour #2425

Merged
merged 4 commits into from
Dec 13, 2023
Merged

Unusual remapping renaming behaviour #2425

merged 4 commits into from
Dec 13, 2023

Conversation

robinhoodie0823
Copy link
Contributor

20231213_034758.mp4

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Dec 13, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: e978ced

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

@robinhoodie0823 robinhoodie0823 linked an issue Dec 13, 2023 that may be closed by this pull request
Unusual Remapping (Renaming) Behaviour #2338
Open
@github-actions GitHub Actions
Copy link
Contributor

PR Analysis

  • 🎯 Main theme: Refactoring the token renaming and remapping functionality in a Figma plugin
  • 📝 PR summary: This PR refactors the token renaming and remapping functionality in a Figma plugin. It modifies the renameGroup function to return the tokens that need to be renamed and then passes these tokens to the remapTokensInGroup function. The PR also updates the relevant test cases to reflect these changes.
  • 📌 Type of PR: Refactor
  • 🧪 Relevant tests added: Yes
  • ⏱️ Estimated effort to review [1-5]: 2, because the changes are straightforward and the PR is not very large.
  • 🔒 Security concerns: No security concerns found

PR Feedback

  • 💡 General suggestions: The PR seems to be well-structured and follows good practices. The changes are clear and the logic seems sound. However, it would be beneficial to add some comments explaining the changes, especially for the renameGroup and remapTokensInGroup functions, as these are key parts of the functionality.

  • 🤖 Code feedback:
    relevant filesrc/app/store/useManageTokens.tsx
    suggestion      Consider adding error handling for the async function calls in `renameGroup` and `remapTokensInGroup` to ensure that any potential errors are caught and handled appropriately. [important]
    relevant lineconst renameGroup = useCallback(async (oldName: string, newName: string, type: string): Promise<{ oldName: string, newName: string }[]> => {
    relevant filesrc/app/store/useTokens.tsx
    suggestion      It might be more efficient to use `Promise.all` to execute the `handleBulkRemap` function calls in parallel, rather than sequentially with `forEach`. [medium]
    relevant linetokensToRename.forEach(async (tokenToRename) => {

How to use

Instructions

To invoke the PR-Agent, add a comment using one of the following commands:
/review: Request a review of your Pull Request.
/describe: Update the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback.
/ask <QUESTION>: Ask a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.
/add_docs: Generate docstring for new components introduced in the PR.
/generate_labels: Generate labels for the PR based on the PR's contents.
see the tools guide for more details.

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

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 13, 2023 •
edited
Loading

Commit SHA:98d1fe0a908a36feb6bb425a8a161e019b3c6dd5

Test coverage results 🧪

Code coverage diff between base branch:main and head branch: 2338-unusual-remapping-renaming-behaviour
Status File % Stmts % Branch % Funcs % Lines
🟢 total 70.9 (0) 61.29 (0) 68.98 (0.02) 71.15 (0)
🟢 src/app/store/useManageTokens.tsx 87.14 (0.19) 80 (0) 83.33 (0) 86.95 (0.19)
🟢 src/app/store/useTokens.tsx 71.08 (0.18) 57.31 (0) 70.58 (0.58) 70.58 (0)
✨ 🆕 src/types/tokens/TokenToRename.ts 100 100 100 100

SorsOps
SorsOps requested changes Dec 13, 2023
View reviewed changes
src/app/components/TokenGroup/TokenGroupHeading.tsx Outdated
renameGroup(path, newTokenGroupName, type);
remapTokensInGroup({ oldGroupName: `${path}.`, newGroupName: `${newTokenGroupName}.`, type });
const tokensToRename = await renameGroup(path, newTokenGroupName, type);
remapTokensInGroup({
Copy link
Member

Choose a reason for hiding this comment

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

This should also have an await in front of it in case someone adds on to this later and wonders why its no executing as expected

src/app/store/useTokens.tsx Outdated
const remapTokensInGroup = useCallback(async ({ oldGroupName, newGroupName, type }: { oldGroupName: string, newGroupName: string, type: string }) => {
const remapTokensInGroup = useCallback(async ({
oldGroupName, newGroupName, type, tokensToRename,
}: { oldGroupName: string, newGroupName: string, type: string, tokensToRename: { oldName: string, newName: string }[] }) => {
Copy link
Member

Choose a reason for hiding this comment

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

Can we make these an actual type instead of inlining in case something else needs it ?

src/app/store/useTokens.tsx Outdated
@@ -227,7 +229,9 @@ export default function useTokens() {
});
if (confirmData && confirmData.result) {
if (Array.isArray(confirmData.data) && confirmData.data.some((data: string) => [UpdateMode.DOCUMENT, UpdateMode.PAGE, UpdateMode.SELECTION].includes(data as UpdateMode))) {
await handleBulkRemap(newGroupName, oldGroupName, confirmData.data[0]);
tokensToRename.forEach(async (tokenToRename) => {
Copy link
Member

Choose a reason for hiding this comment

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

This could cause an issue with race conditions if the bulk remap is slow and something else attempts to change data while its transacting.

Lets change this to

  await Promise.all( tokensToRename.map(tokenToRename) =>  handleBulkRemap(tokenToRename.newName, tokenToRename.oldName, confirmData.data[0]));
)

@keeganedwin
Copy link
Collaborator

keeganedwin commented Dec 13, 2023 •
edited
Loading

Tested and confirming that this is fixed @esthercheran

@SorsOps SorsOps merged commit d44d993 into main Dec 13, 2023
@SorsOps SorsOps deleted the 2338-unusual-remapping-renaming-behaviour branch December 13, 2023 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SorsOps SorsOps SorsOps approved these changes

@six7 six7 Awaiting requested review from six7

@esthercheran esthercheran Awaiting requested review from esthercheran

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unusual Remapping (Renaming) Behaviour
3 participants
@robinhoodie0823 @keeganedwin @SorsOps