Skip to content

Conversation

@panteliselef
Copy link
Member

@panteliselef panteliselef commented Jan 27, 2025

Description

Managing urls to dashboard like that is not sustainable, this will be addressed the following PR, once the flow has been finalized

Checklist

  • pnpm test runs as expected.
  • pnpm build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

@panteliselef panteliselef requested review from a team and kaftarmery January 27, 2025 10:06
@panteliselef panteliselef self-assigned this Jan 27, 2025
@vercel
Copy link

vercel bot commented Jan 27, 2025

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

Name Status Preview Comments Updated (UTC)
clerk-js-sandbox ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 27, 2025 0:52am

@changeset-bot
Copy link

changeset-bot bot commented Jan 27, 2025

🦋 Changeset detected

Latest commit: ccc95c2

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

This PR includes changesets to release 3 packages
Name Type
@clerk/clerk-js Patch
@clerk/chrome-extension Patch
@clerk/clerk-expo 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

Copy link
Contributor

@LekoArts LekoArts left a comment

Choose a reason for hiding this comment

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

Please add a PR description, thanks!

);
return url.href;
} catch {
return '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to use the last-active path as fallback instead of an empty string?

Copy link
Member Author

Choose a reason for hiding this comment

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

this handles the case of the the copyKeysUrl being malformed. I wanted to avoid using dashboad.clerk.com/last-active in general, but maybe i will added it once i have a proper solution to handle dashboard navigations from fapi/worker.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see 👍 I guess my point was that after copyKeysUrl is malformed and then also the regex fails, should we have a fallback that "does something" (because empty string won't do anything and the user also doesn't know what's wrong) or should we throw an error? We probably don't want to show an error since it's not actionable (they can't do anything about it) but silently failing is also not nice.

Not familiar with all moving pieces here so I'll trust your judgement :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's having something more solid and then iterate. I've pushed this

@panteliselef panteliselef merged commit 93ae27c into main Jan 27, 2025
29 checks passed
@panteliselef panteliselef deleted the elef/actls-71-update-dashboard-link-to-go-directly-to-application branch January 27, 2025 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants