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

Feat: #482 when Reset topic clicked it should delete comments too #580

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Feat: #482 when Reset topic clicked it should delete comments too #580

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
51b9710
feat: #482 when Reset topic clicked it should delete comments too
2div Dec 1, 2024
d676d85
Merge branch 'amelioro:main' into main
2div Dec 1, 2024
44653c5
Merge branch 'amelioro:main' into main
2div Dec 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 8 additions & 11 deletions src/web/comment/store/commentStore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
import { emitter } from "@/web/common/event";
import { storageWithDates } from "@/web/common/store/utils";
import { StoreTopic, UserTopic } from "@/web/topic/store/store";
import { setSelected } from "@/web/view/currentViewStore/store";

Check failure on line 12 in src/web/comment/store/commentStore.ts

View workflow job for this annotation

GitHub Actions / lint 

There should be at least one empty line between import groups
import { toggleShowResolvedComments } from "@/web/view/miscTopicConfigStore";
import { useState } from "react";

Check failure on line 13 in src/web/comment/store/commentStore.ts

View workflow job for this annotation

GitHub Actions / lint 

`react` import should occur before import of `short-uuid`

Check failure on line 13 in src/web/comment/store/commentStore.ts

View workflow job for this annotation

GitHub Actions / lint 

'useState' is defined but never used. Allowed unused vars must match /^_/u
keyserj marked this conversation as resolved.
Show resolved Hide resolved

export type StoreComment = Omit<Comment, "topicId">;

Expand Down Expand Up @@ -160,6 +160,10 @@
);
};

export const resetComment = () => {
useCommentStore.setState({ ...initialState, comments: [] }, false, "resetComment");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here is the main cause of the error you're seeing: we want to reset to the initial state except for the initial state's topic value, similar to how resetTopicData() is implemented. This is from an unfortunate bit of tech debt 😅 I'm hoping to remove it soon in #581. Essentially if you reset the Topic, we won't know which Topic to make an API request for, preventing the changes from being saved properly.

Side note: you also don't need to set comments: [] here - initialState has that in it already.

};

export const resolveComment = (commentId: string, resolved: boolean) => {
useCommentStore.setState(
(state) => ({
Expand All @@ -174,9 +178,8 @@

export const loadCommentsFromApi = (topic: UserTopic, comments: StoreComment[]) => {
const builtPersistedName = `${persistedNameBase}-user`;
useCommentStore.persist.setOptions({ name: builtPersistedName });

useCommentStore.apiSyncer.pause();
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are changes I made in a separate commit - it seems that when you rebased, you removed these changes. This might separately cause problems for you. The easiest way to fix this right now is probably to just manually add these lines back in, and create a fix commit with that.

What I suspect happened is that when you pulled and saw conflicts, you clicked "accept current changes", which kept your changes and removed the changes being pulled in. In this case, you'd have wanted to keep both your changes and the changes being pulled in (generally if you pull changes that someone else made, you probably want to keep them).

To avoid this in the future, it might help to read up on handling merge conflicts and why/how to rebase. Personally, I think Philomatics does a fantastic job of explaining these concepts:

Additionally, I would fixup (i.e. squash without rewording the original commit's message) said fix commit into your original commit, so that your commit only has your changes, and the commit history is a little cleaner. But you don't have to fixup because I'll handle squashing all commits before merging anyway. If you'd like to look into this, here's another great Philomatics video explaining the interactive rebase, which makes it easy to modify your commit history.

Copy link
Author

Choose a reason for hiding this comment

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

Hey @keyserj , really appreciate you sharing the Philomatics YouTube channel. It was super helpful!

In my last commit, I made a pull request, then committed my changes, and pushed them.

I think you’re right about your note. When I did the pull request, I hit "accept current changes" because I wanted to include my changes, thinking that the other changes would merge in too.

Copy link
Author

Choose a reason for hiding this comment

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

The easiest way to fix this right now is probably to just manually add these lines back in, and create a fix commit with that.

Are you suggesting I should edit the git files in the browser or in VSCode and then push it again?

Copy link
Collaborator

Choose a reason for hiding this comment

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

When I did the pull request, I hit "accept current changes" because I wanted to include my changes, thinking that the other changes would merge in too.

One tip I have is to look at the diff between your branch and the upstream after you've resolved conflicts - the diff should only have your changes, not anyone else's (unless you're aware of their changes and intentionally want to change them). You can use a PR to see this diff or use vscode's gitlens extension to compare HEAD (your latest commit on your checked out branch) with upstream/main to see the same without a PR, like so:

Code_6OHRbZCwXk.mp4

Are you suggesting I should edit the git files in the browser or in VSCode and then push it again?

Generally I would make changes locally (via vscode) so that I can test them before pushing anyway (I'm also less familiar with the browser interface). But in either case you'll need to create a new commit for it (or modify the existing commit, but that's not necessary). Also note that if you use the browser, you'll need to remember to pull those changes locally if you do any further local development.

useCommentStore.persist.setOptions({ name: builtPersistedName });

useCommentStore.setState(
{
Expand All @@ -202,24 +205,19 @@
true,
"loadCommentsFromApi",
);

useCommentStore.apiSyncer.resume();
};

export const loadCommentsFromLocalStorage = async () => {
const builtPersistedName = `${persistedNameBase}-playground`;
useCommentStore.persist.setOptions({ name: builtPersistedName });

useCommentStore.apiSyncer.pause();
useCommentStore.persist.setOptions({ name: builtPersistedName });

if (useCommentStore.persist.getOptions().storage?.getItem(builtPersistedName)) {
// TODO(bug): for some reason, this results in an empty undo action _after_ clear() is run - despite awaiting this promise
await useCommentStore.persist.rehydrate();
} else {
useCommentStore.setState(initialState, true, "loadCommentsFromLocalStorage");
}

useCommentStore.apiSyncer.resume();
};

export const viewComment = (commentId: string) => {
Expand All @@ -235,10 +233,9 @@
if (!threadStarterComment)
throw errorWithData("couldn't find thread-starter comment", comment.parentId, state.comments);

if (threadStarterComment.resolved) toggleShowResolvedComments(true); // otherwise going to comment via URL won't show it if it's resolved

const commentParentGraphPartId =
threadStarterComment.parentType === "topic" ? null : threadStarterComment.parentId;

setSelected(commentParentGraphPartId);
emitter.emit("viewTopicDetails");
};
59 changes: 56 additions & 3 deletions src/web/topic/components/TopicWorkspace/MoreActionsDrawer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,21 @@
ToggleButton,
Tooltip,
Typography,
Button,

Check failure on line 34 in src/web/topic/components/TopicWorkspace/MoreActionsDrawer.tsx

View workflow job for this annotation

GitHub Actions / lint 

Member 'Button' of the import declaration should be sorted alphabetically
Dialog,
DialogActions,
DialogContent,
DialogContentText,
DialogTitle,
} from "@mui/material";
import { toPng } from "html-to-image";
import { getRectOfNodes, getTransformForBounds } from "reactflow";

import { NumberInput } from "@/web/common/components/NumberInput/NumberInput";
import { getDisplayNodes } from "@/web/topic/components/Diagram/externalFlowStore";
import {
getDisplayNodes,
setDisplayNodesGetter,

Check failure on line 47 in src/web/topic/components/TopicWorkspace/MoreActionsDrawer.tsx

View workflow job for this annotation

GitHub Actions / lint 

'setDisplayNodesGetter' is defined but never used. Allowed unused vars must match /^_/u
} from "@/web/topic/components/Diagram/externalFlowStore";
import { downloadTopic, uploadTopic } from "@/web/topic/loadStores";
import { useOnPlayground } from "@/web/topic/store/topicHooks";
import { resetTopicData } from "@/web/topic/store/utilActions";
Expand Down Expand Up @@ -75,12 +84,14 @@
useFormat,
} from "@/web/view/currentViewStore/store";
import { resetQuickViews } from "@/web/view/quickViewStore/store";
import { resetComment } from "@/web/comment/store/commentStore";

Check failure on line 87 in src/web/topic/components/TopicWorkspace/MoreActionsDrawer.tsx

View workflow job for this annotation

GitHub Actions / lint 

`@/web/comment/store/commentStore` import should occur before import of `@/web/common/components/NumberInput/NumberInput`
import {

Check failure on line 88 in src/web/topic/components/TopicWorkspace/MoreActionsDrawer.tsx

View workflow job for this annotation

GitHub Actions / lint 

There should be at least one empty line between import groups
toggleFillNodesWithColor,
toggleIndicateWhenNodeForcedToShow,
useFillNodesWithColor,
useIndicateWhenNodeForcedToShow,
} from "@/web/view/userConfigStore";
import { useState } from "react";

Check failure on line 94 in src/web/topic/components/TopicWorkspace/MoreActionsDrawer.tsx

View workflow job for this annotation

GitHub Actions / lint 

`react` import should occur before import of `reactflow`

const imageWidth = 2560;
const imageHeight = 1440;
Expand Down Expand Up @@ -149,6 +160,48 @@

const fillNodesWithColor = useFillNodesWithColor();
const indicateWhenNodeForcedToShow = useIndicateWhenNodeForcedToShow();
const [resetDialogOpen, setResetDialogOpen] = useState(false);

const resetComments = (
<Dialog
open={resetDialogOpen}
onClose={() => setResetDialogOpen(false)}
aria-labelledby="alert-dialog-title"
>
<DialogTitle id="alert-dialog-title">
{/* Reset Comment {user.username}/{topic.title}? */}
Reset Comments ?
</DialogTitle>
<DialogContent>
<DialogContentText>
This topic has comments that will be deleted by this action, and undo will not restore
these comments. Continue resetting?
</DialogContentText>
</DialogContent>
<DialogActions>
<Button
color="inherit"
onClick={() => {
setResetDialogOpen(false);
}}
>
Cancel
</Button>
<Button
color="error"
variant="contained"
onClick={() => {
setResetDialogOpen(false);
resetComment();
resetTopicData();
resetQuickViews();
}}
>
RESET COMMENT
</Button>
</DialogActions>
</Dialog>
);

return (
<Drawer
Expand Down Expand Up @@ -203,8 +256,7 @@
title="Reset Topic"
aria-label="Reset Topic"
onClick={() => {
resetTopicData();
resetQuickViews();
setResetDialogOpen(true);
// intentionally not resetting comments/drafts, since undoing a reset would be painful if comments were lost,
// and it's annoying to try and put these all on the same undo/redo button.
// if orphaned comments are really a problem, we should be able to manually clean them up.
Expand All @@ -213,6 +265,7 @@
>
<AutoStoriesOutlined />
</IconButton>
{resetDialogOpen ? resetComments : <></>}
</>
)}

Expand Down
Loading