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

User's Shared Chats directly accessibly from UI again #506

Merged
merged 7 commits into from
Mar 21, 2024

Conversation

rjwignar
Copy link
Collaborator

@rjwignar rjwignar commented Mar 16, 2024

This quick and dirty PR fixes #480

Current Shared Chat Access Flow on ChatCraft (3/15/2024)

We know that currently, a shared chat in the Shared Chats accordion redirects to a url via the /api/share prefix, e.g. [website-url]/api/share/{user}/{id}:

Current Share URL format (/api/share) in Shared Chats Accordion

image

Current Error when Accessing Shared Chat from Sidebar Accordion redirect url

Clicking the Shared Chat to access this url fails and returns two 404 errors (likely a react-router-dom issue based on findings in this week's triage meeting):
image

Successfully loading the Accordion redirect url in address bar

However, copy/pasting the this link into the address bar will redirect it to the /c/ prefix equivalent, e.g [website-url]/c/{user}/{id} (keep an eye on the address bar in this gif):
issue480newApiRoute

Previous Shared Chat Access Flow

After a couple days of comparing the main branch to past branches that didn't have this issue, clicking a shared chat from the Shared Chats accordion would redirect to a shared chat directly via the /c/ prefix, e.g. [website-url]/c/{user}/{id}:
Previous Share URL format (/c/
image.

Compare this to a current Shared Chat item url using the /api/share/ route, which, while accessible by copy/pasting the link into a new tab, will crash ChatCraft when accessed through the Shared Chats accordion UI.

#417 added createDataShareUrl(), which uses createShareUrl() to create a Shared Chat with the shared chat url with the /api/share/{user}/id format instead of /c/{user}/{id}:
image

This is why we had conflicting reports of this issue.
Those who reported the issue were accessing recently-created Shared Chats (and given given the /api/share/-style redirect URL in the UI), while those who didn't see the issue were accessing Shared Chats that were created before this change (and given the /c/-style redirect URL in the UI)

My Solution (not ideal)

createDataShareUrl() replaced createShareUrl() in src/lib/ChatCraftChat.ts when creating a Shared Chat:
image

This PR fixes the shared-chat access issue by replacing createDataShareUrl() calls with createShareUrl() calls in src/lib/ChatCraftChat.ts to give Shared Chat items the /c/{user}/{id}-style redirect URLs instead of the /api/share/{user}/{id}-style redirect URLs.

I don't think this is a perfect solution and it's not my preferred solution.
For one, this change still doesn't allow users to access their own shared chats created in the last month (at least not through the UI)
I'd rather find a way to use the /api/share/{user}/{id}-style URLs directly from the UI, but this is all I can come up with for now

@rjwignar
Copy link
Collaborator Author

rjwignar commented Mar 16, 2024

Hold up, I might have another fix.
Update: Nevermind, I thought I found out why the /api/share/{user}/{id} URL doesn't work when used in a UI component but my tests didn't work. This PR is the best fix I have for now.

@rjwignar rjwignar changed the title reverted createDataShareUrl() calls to createShareUrl() User's Shared Chats directly accessibly from UI again Mar 16, 2024
Copy link

cloudflare-workers-and-pages bot commented Mar 17, 2024

Deploying chatcraft-org with  Cloudflare Pages  Cloudflare Pages

Latest commit: dcf3897
Status: ✅  Deploy successful!
Preview URL: https://c5fe9a50.console-overthinker-dev.pages.dev
Branch Preview URL: https://rjwignar-issue-480.console-overthinker-dev.pages.dev

View logs

@humphd
Copy link
Collaborator

humphd commented Mar 20, 2024

What's the status on this? Is this fix correct or still an experiment?

@rjwignar
Copy link
Collaborator Author

What's the status on this? Is this fix correct or still an experiment?

If this is merged, shared chats created afterwards will be accessible from the UI, but the ones created until now still won't be (unless users copy/paste the "/api/share"-style URL in their browser's address bar.

I'm still figuring out a way to make those chats accessible from the UI

@rjwignar rjwignar marked this pull request as draft March 20, 2024 20:06
@rjwignar
Copy link
Collaborator Author

New Changes

I've updated this PR with a different set of changes.
Originally, I modified the SharedChatCraftChat creation process to once again give the "/c"-style URLs to the created object instead of the "/api/share"-style URLs, but this didn't fix the Shared Chats already created with the "api/share"-style URLs.

These changes add a new method to src/lib/share.ts, convertToShareUrl(), which returns a "/c"-style URL if the input URL is a "/api/share"-style URL.
convertToShareUrl() is then used to ensure each SharedChatCraftChat's "/c"-style URL is used when rendering them as ChatSideBarItem objects

Explanation for Changes

In today's meeting, we discussed two possible solutions to make the old Shared Chats with a "/api/share"-style URL accessible again:

  1. Render the Shared Chat's "/c"-style URL instead of the "/api/share"-style URL
  2. Use window.location to bypass react-router-dom and redirect to the Shared Chats' "/c"-style URL

I elected to implement Solution 1 as I believe implementing this solution is less invasive than implementing Solution 2
(I'm conditionally modifying the SharedChat URL used when rendering Shared Chats (as ChatSidebarItem objects) instead of modifying the ChatSidebarItem component itself).

Where are your changes to src/lib/ChatCraftChat.ts?

Either solution fixes the issue of accessing Shared Chats that were given a "/api/share"-style URL, meaning there's no need to modify how new SharedChatCraftChat's are created. I removed the original changes I made in this PR.

This means created Shared Chats will still be created with an "/api/share"-style URL.
The new changes just mean they will be rendered in the UI with their "/c"-style URL.

How do I test this?

On this branch's preview build, start a new conversation and share two things:

  • A single message within the Chat
  • The entire Chat itself

You should be able to access both the newly created Shared Chat and the Shared Message

Please let me know if this PR requires any changes.
I placed convertToShareUrl() inside src/lib/share.ts as it felt appropriate to place it near createDataShareUrl() and createShareUrl(), but maybe it should be moved elsewhere.
Regardless, I'm happy to share this PR as this bug has been bothering us for weeks.

@rjwignar rjwignar marked this pull request as ready for review March 20, 2024 22:39
@rjwignar rjwignar requested a review from Rachit1313 March 20, 2024 22:40
@rjwignar rjwignar self-assigned this Mar 21, 2024
@kliu57 kliu57 self-requested a review March 21, 2024 19:45
Copy link
Collaborator

@kliu57 kliu57 left a comment

Choose a reason for hiding this comment

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

Just tested it on your preview url and works well. Where I was getting an error page by clicking a shared chat from sidebar in production, I am no longer getting any error when doing the same action in your preview url. Tested it with chrome and firefox.

@rjwignar rjwignar merged commit 42f7efe into main Mar 21, 2024
4 checks passed
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.

Chatcraft crashes when trying to access shared chats
4 participants