Third bunch of possible undefined thoughts #2744

snqb · 2024-12-28T23:47:24Z

snqb · 2024-12-28T23:56:45Z

src/selectors/parentOfThought.ts

@@ -8,7 +8,7 @@ const parentOfThought = (state: State, thoughtId: ThoughtId): Thought | null =>
  const thought = getThoughtById(state, thoughtId)
  if (!thought) return null
  const parentThought = getThoughtById(state, thought.parentId)
-  return parentThought
+  return parentThought ?? null


preserve fn signature

snqb · 2024-12-29T00:01:38Z

src/stores/commandStateStore.ts

@@ -36,7 +36,7 @@ export const updateCommandState = () => {
  const action =
    selection.isActive() && selection.isOnThought()
      ? getCommandState(selection.html() ?? '')
-      : getCommandState(pathToThought(state, state.cursor).value)
+      : getCommandState(pathToThought(state, state.cursor)?.value ?? '')


on that end we have a path.length === 0 default case, so fallback is fine here

snqb · 2024-12-29T00:05:21Z

src/util/importJSON.ts

-  const rankStart = destEmpty ? destThought.rank : getNextRank(state, head(simplePath))
-  const rankIncrement = getRankIncrement(state, blocks, destThought, rankStart)
+  const rankStart = destEmpty ? destThought?.rank : getNextRank(state, head(simplePath))
+  const rankIncrement = destEmpty ? getRankIncrement(state, blocks, destThought, rankStart) : 1


1 is a fallback in getRankIncrement too, so that's my judgment

snqb · 2024-12-29T00:07:35Z

src/test-helpers/deleteThoughtAtFirstMatch.ts

@@ -19,6 +19,8 @@ const getThoughtAndParentPath = (state: State, at: string[]): [Thought, Path] =>

  const thought = pathToThought(state, path)

+  if (!thought) throw new Error(`Thought not found for path: ${path}`)


it's a test file, and we throw an error for empty path above, so thought why not

snqb · 2024-12-29T00:08:26Z

src/test-helpers/contextToThought.ts

@@ -9,7 +9,7 @@ import getThoughtById from '../selectors/getThoughtById'
 */
 const contextToThought = (state: State, context: Context): Thought | null => {
  const id = contextToThoughtId(state, context)
-  return id ? getThoughtById(state, id) : null
+  return id ? (getThoughtById(state, id) ?? null) : null


to preserve type for now

but there's an itch to make it Thought | null | undefined
wdyt?

I would do Thought | null or Thought | undefined since we don't need to distinguish between null and undefined here.

ok, I made it Thought | undefined, and it broke nothing

(it broke one test :D)

raineorshine

Thanks!

raineorshine · 2024-12-29T17:46:34Z

src/actions/formatLetterCase.ts

-    const originalThoughtValue = thought.value
+    const originalThoughtValue = thought?.value
+
+    if (originalThoughtValue === undefined) return


I think this might be a bit more direct here (above the originalThoughtValue declaration):

if (!thought) return

I'm not sure if I got you right, but here's what I've ended up with

const cursor = state.cursor if (!cursor) return const originalThoughtValue = pathToThought(state, cursor)?.value if (originalThoughtValue === undefined) return

originalThoughtValue can only be undefined due to the optional chaining operator. It doesn't really make sense to use it here in order to check for undefined, when what you really want to check is if the thought is undefined. thought.value is always a string. There's no reason to get it mixed up with the undefined thought check.

const thought = pathToThought(state, cursor) if (!thought) return

raineorshine · 2024-12-29T17:49:13Z

src/hooks/useDragAndDropSubThought.ts

-      newRank: (dropTop ? getPrevRank : getNextRank)(state, thoughtTo.id),
-    }),
-  )
+  if (thoughtTo) {


I would prefer if (!thoughtTo) return or combining it with the "cannot drop on itself" short circuit.

raineorshine · 2024-12-29T17:54:55Z

src/initialize.ts

-    return contexts.map(id => thoughtToContext(state, getThoughtById(state, id)?.parentId))
+    return contexts
+      .map(id => getThoughtById(state, id))
+      .filter(Boolean)
+      .map(context => context.parentId)


I think thoughtToContext got lost here, altering the previous behavior.

raineorshine · 2024-12-29T17:56:56Z

src/selectors/prevSibling.ts

-  const siblings = showContexts
-    ? getContextsSortedAndRanked(state, getThoughtById(state, head(parentPath)).value)
-    : getChildrenSorted(state, thought.parentId)
+  const siblings =
+    showContexts && parent
+      ? getContextsSortedAndRanked(state, parent.value)
+      : getChildrenSorted(state, thought.parentId)


This will incorrectly return the siblings from the normal view if parent is missing. It would be better to return [] when showContexts is true and the parent is missing.

raineorshine · 2024-12-29T17:59:35Z

src/test-helpers/contextToThought.ts

@@ -9,7 +9,7 @@ import getThoughtById from '../selectors/getThoughtById'
 */
 const contextToThought = (state: State, context: Context): Thought | null => {
  const id = contextToThoughtId(state, context)
-  return id ? getThoughtById(state, id) : null
+  return id ? (getThoughtById(state, id) ?? null) : null


I would do Thought | null or Thought | undefined since we don't need to distinguish between null and undefined here.

raineorshine · 2024-12-29T18:01:28Z

src/util/importJSON.ts

@@ -63,6 +63,7 @@ const insertThought = (
  },
 ) => {
  const thoughtOld = getThoughtById(state, id)
+  if (!thoughtOld) return


Let's return null here since insertUpdates is nullable below.

raineorshine · 2025-01-03T21:01:14Z

src/hooks/useDragAndDropSubThought.ts

+  if (!thoughtTo) {
+    console.warn(`Cannot drop ${thoughtFrom} on itself. Aborting drop.`)
+    return
+  }


This console.warn is not accurate, as dropping a thought on itself is already detected and short circuited using the equalPath check above. Please update the message or remove the console.warn completely, combining it with the first return statement.

raineorshine · 2025-01-03T21:02:46Z

src/hooks/useDragAndDropSubThought.ts

@@ -134,7 +139,7 @@ const drop = (props: DroppableSubthoughts, monitor: DropTargetMonitor) => {
  )

  // alert user of move to another context
-  if (!sameContext) {
+  if (!sameContext && thoughtTo && thoughtFrom) {


This isn't necessary due to the short circuit above.

third bunch

b4decc7

snqb marked this pull request as draft December 28, 2024 23:55

snqb commented Dec 28, 2024

View reviewed changes

fix prev sibling

9fa99ad

snqb commented Dec 29, 2024

View reviewed changes

rewrite deleteThoughtAtFirstMatch

ce82e0a

snqb commented Dec 29, 2024

View reviewed changes

snqb marked this pull request as ready for review December 29, 2024 00:12

raineorshine requested changes Dec 29, 2024

View reviewed changes

trevinhofmann assigned snqb Dec 31, 2024

address feedback

01b8c52

snqb requested a review from raineorshine January 3, 2025 11:41

null -> undefined for contextToThought test

d77905b

trevinhofmann assigned trevinhofmann and unassigned snqb Jan 3, 2025

trevinhofmann self-requested a review January 3, 2025 20:12

raineorshine requested changes Jan 3, 2025

View reviewed changes

minor refactor

5c816ff

raineorshine merged commit 0b0697d into cybersemics:main Jan 3, 2025
3 checks passed

trevinhofmann removed their assignment Jan 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Third bunch of possible undefined thoughts #2744

Third bunch of possible undefined thoughts #2744

snqb commented Dec 28, 2024 •

edited by raineorshine

Loading

snqb Dec 28, 2024

snqb Dec 29, 2024

snqb Dec 29, 2024

snqb Dec 29, 2024

snqb Dec 29, 2024

raineorshine Dec 29, 2024

snqb Jan 3, 2025 •

edited

Loading

raineorshine left a comment

raineorshine Dec 29, 2024

snqb Jan 3, 2025

raineorshine Jan 3, 2025 •

edited

Loading

raineorshine Dec 29, 2024

raineorshine Dec 29, 2024

raineorshine Dec 29, 2024

raineorshine Dec 29, 2024

raineorshine Dec 29, 2024

raineorshine Jan 3, 2025

raineorshine Jan 3, 2025

		@@ -19,6 +19,8 @@ const getThoughtAndParentPath = (state: State, at: string[]): [Thought, Path] =>

		const thought = pathToThought(state, path)

		if (!thought) throw new Error(`Thought not found for path: ${path}`)

Third bunch of possible undefined thoughts #2744

Third bunch of possible undefined thoughts #2744

Conversation

snqb commented Dec 28, 2024 • edited by raineorshine Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snqb Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

raineorshine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raineorshine Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snqb commented Dec 28, 2024 •

edited by raineorshine

Loading

snqb Jan 3, 2025 •

edited

Loading

raineorshine Jan 3, 2025 •

edited

Loading