Skip to content

feat: rename workspace #163

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 2 commits into from
Jan 22, 2025
Merged

feat: rename workspace #163

merged 2 commits into from
Jan 22, 2025

Conversation

peppescg
Copy link
Collaborator

Kapture.2025-01-22.at.10.27.10.mp4
Screenshot 2025-01-22 at 10 27 37

@peppescg peppescg self-assigned this Jan 22, 2025
@peppescg peppescg linked an issue Jan 22, 2025 that may be closed by this pull request
Copy link
Member

@kantord kantord left a comment

Choose a reason for hiding this comment

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

Great job!

The only thing I'm slightly missing is a test that actually tests the process of renaming using an MSW handler for the relevant endpoint as well.

But there is no reason to block a release because of that at all

Comment on lines +26 to +30
const handleSubmit = (e: FormEvent<HTMLFormElement>) => {
e.preventDefault();
mutate({ body: { name: workspaceName, rename_to: name } });
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a hook in the system prompt editor useSavedStatus

function useSavedStatus() {
const [saved, setSaved] = useState<boolean>(false);
useEffect(() => {
const id = setTimeout(() => setSaved(false), 2000);
return () => clearTimeout(id);
}, [saved]);
return { saved, setSaved };
}

We could re-use it here to show a pending+confirmation state

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mm I saw it, honestly I don't see the value 🤔
I mean we have isPending and the onSuccess callback for notification, why we need this hook?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to provide visual feedback that it has been saved — also for consistency.
Feel free to merge without this — I would like to go back and standardise it - but it's deffo not a blocker.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean, I would prefer to show a notification after a mutation, I would standardise the notification approach after a mutation...wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a valid option too

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's merge as is and standardise on something in next iteration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

makes sense 👍

@peppescg
Copy link
Collaborator Author

Great job!

The only thing I'm slightly missing is a test that actually tests the process of renaming using an MSW handler for the relevant endpoint as well.

But there is no reason to block a release because of that at all

yep, let me add it in a next PR, avoid block the release if it makes sense to you

@kantord
Copy link
Member

kantord commented Jan 22, 2025

Great job!
The only thing I'm slightly missing is a test that actually tests the process of renaming using an MSW handler for the relevant endpoint as well.
But there is no reason to block a release because of that at all

yep, let me add it in a next PR, avoid block the release if it makes sense to you

exactly, that is what I'm thinking

@coveralls
Copy link
Collaborator

coveralls commented Jan 22, 2025

Pull Request Test Coverage Report for Build 12905363452

Details

  • 7 of 9 (77.78%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 69.048%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/features/workspace/components/workspace-name.tsx 4 6 66.67%
Totals Coverage Status
Change from base Build 12905054401: -0.02%
Covered Lines: 604
Relevant Lines: 783

💛 - Coveralls

@peppescg peppescg merged commit 3b68754 into main Jan 22, 2025
7 of 8 checks passed
@peppescg peppescg deleted the issues/155 branch January 22, 2025 09:52
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.

Add rename workspace
4 participants