Add reset function to useWebViewState

[rolfheij-sil]

This change is 

[irahopkinson]

This looks fine to me but I'm not familiar enough with this part of the code. @tjcouch-sil can you review this also?

Reviewed 2 of 8 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @rolfheij-sil)

---

src/renderer/hooks/use-web-view-state.hook.ts line 22 at r2 (raw file):

  }, [defaultStateValue, state, stateKey]);

  const resetState = () => {

Does this need to be enclosed in a useCallback?

---

src/renderer/services/web-view-state.service.ts line 107 at r2 (raw file):

 */
export function resetWebViewStateById(id: string, stateKey: string): void {
  if (!id || !stateKey) throw new Error('id and stateKey must be provided to remove webview state');

Probably should use .trim() on each of these inputs to catch strings with no useful content.

[tjcouch-sil]

Reviewed 6 of 8 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @irahopkinson and @rolfheij-sil)

---

src/renderer/hooks/use-web-view-state.hook.ts line 14 at r2 (raw file):

  defaultStateValue: T,
): [webViewState: T, setWebViewState: Dispatch<SetStateAction<T>>, resetWebViewState: () => void] {
  const [state, setState] = useState(() => this.getWebViewState(stateKey) ?? defaultStateValue);

This will need to be updated to pass the default value into getWebViewState

---

src/renderer/hooks/use-web-view-state.hook.ts line 22 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Does this need to be enclosed in a useCallback?

This does indeed need to be enclosed in a useCallback to keep the returned function stable :)

---

src/renderer/services/web-view-state.service.ts line 74 at r2 (raw file):

 * @returns String (if it exists) containing the state for the given key of the given web view
 */
export function getWebViewStateById<T>(id: string, stateKey: string): T | undefined {

You need to add a defaultValue: T parameter on here in accord with the new get/set/reset pattern since default values are not stored elsewhere. Then you can remove | undefined on the return type.

Note: since defaultValue is a required parameter, you can always trust it as the correct defaultValue if there isn't a value at the key specified; you do not need to throw. I didn't realize it before, but it is also the case with getSetting in the setting service. Sorry I didn't think about it before. I can update it unless you particularly desire to do so since I will already be in that area of the code with my hook work. Also the @return JSDoc on getSetting is out of date. undefined is no longer returned arbitrarily (but it should be returned if defaultSetting is undefined and there isn't another value).

[tjcouch-sil]

Thanks for the good work on this! Looking forward to having this in soon :)

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @irahopkinson and @rolfheij-sil)

[irahopkinson]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @rolfheij-sil)

---

src/renderer/services/web-view-state.service.ts line 107 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Probably should use .trim() on each of these inputs to catch strings with no useful content.

I talked about this with @tjcouch-sil and since this is just copying an existing code example, and fixing this here doesn't fix it everywhere else, I'm making this non-blocking.

[rolfheij-sil]

Thanks!

Reviewable status: 0 of 9 files reviewed, 4 unresolved discussions (waiting on @irahopkinson and @tjcouch-sil)

---

src/renderer/hooks/use-web-view-state.hook.ts line 14 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

This will need to be updated to pass the default value into getWebViewState

Done.

---

src/renderer/hooks/use-web-view-state.hook.ts line 22 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

This does indeed need to be enclosed in a useCallback to keep the returned function stable :)

Done.

---

src/renderer/services/web-view-state.service.ts line 74 at r2 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

You need to add a defaultValue: T parameter on here in accord with the new get/set/reset pattern since default values are not stored elsewhere. Then you can remove | undefined on the return type.

Note: since defaultValue is a required parameter, you can always trust it as the correct defaultValue if there isn't a value at the key specified; you do not need to throw. I didn't realize it before, but it is also the case with getSetting in the setting service. Sorry I didn't think about it before. I can update it unless you particularly desire to do so since I will already be in that area of the code with my hook work. Also the @return JSDoc on getSetting is out of date. undefined is no longer returned arbitrarily (but it should be returned if defaultSetting is undefined and there isn't another value).

Done. Thanks for thinking all that through. I updated the code.

---

src/renderer/services/web-view-state.service.ts line 107 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I talked about this with @tjcouch-sil and since this is just copying an existing code example, and fixing this here doesn't fix it everywhere else, I'm making this non-blocking.

I think I'll leave this for now. Like you say, just fixing it in this one place doesn't really improve things

[tjcouch-sil]

thanks for the hard work!

Reviewed 9 of 9 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

---

src/renderer/hooks/use-web-view-state.hook.ts line 20 at r3 (raw file):

  useEffect(() => {
    if (
      this.getWebViewState(stateKey, defaultStateValue) === defaultStateValue &&

Because I know you love to learn, I will put some deeper-level comments here. To be clear, I don't expect you to make these changes. Just notes so you can learn :) if you really want to implement these changes, of course, feel free. But I just want to be clear that I do not expect you to make these changes at all. They are informational only.

I'm not exactly sure of the purpose for this if statement, but I suspect this useEffect could be rewritten to simplify it. I think the idea here is "if the default state changes, get the new state and update our state internally". If I'm not missing something, I believe you could rewrite this useEffect like this:

useEffect(() => {
  setStateInternal(this.getWebViewState(stateKey, defaultStateValue));
}, [defaultStateValue, stateKey]);

useState's set function internally compares the current value to the new value and doesn't update or trigger re-renders if they are referentially equal (===), so we don't need to avoid setting if they are equal. I believe that removes the need for state in the dependency array, which I believe removes the need for the first part of the if statement (not sure, though; again, I'm not sure of the purpose for that part) to avoid circular re-renders. We can trust this.getWebViewState will give us the appropriate value in any situation. As such, I think this rework would solve the problems here. I believe it also achieves keeping the state up-to-date when the key changes, which I do not believe will happen currently.

I will probably make some adjustments that include this in my PR FYI.

Again, this is just informational. No action needed unless you particularly desire to make the change yourself!

---

src/renderer/hooks/use-web-view-state.hook.ts line 28 at r3 (raw file):

  const setState = useCallback(
    (newStateValue: T) => {
      setStateInternal(newStateValue);

Learning disclaimer I think I would probably put setStateInternal after this.setWebViewState so that, if setWebViewState throws, setStateInternal doesn't incorrectly have the new value. Same with reset below.

---

src/renderer/services/web-view-state.service.ts line 107 at r2 (raw file):

Previously, rolfheij-sil wrote…

I think I'll leave this for now. Like you say, just fixing it in this one place doesn't really improve things

I am resolving this discussion to avoid blocking the PR.

[rolfheij-sil]

Reviewable status: complete! all files reviewed, all discussions resolved

---

src/renderer/hooks/use-web-view-state.hook.ts line 20 at r3 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Because I know you love to learn, I will put some deeper-level comments here. To be clear, I don't expect you to make these changes. Just notes so you can learn :) if you really want to implement these changes, of course, feel free. But I just want to be clear that I do not expect you to make these changes at all. They are informational only.

I'm not exactly sure of the purpose for this if statement, but I suspect this useEffect could be rewritten to simplify it. I think the idea here is "if the default state changes, get the new state and update our state internally". If I'm not missing something, I believe you could rewrite this useEffect like this:

useEffect(() => {
  setStateInternal(this.getWebViewState(stateKey, defaultStateValue));
}, [defaultStateValue, stateKey]);

useState's set function internally compares the current value to the new value and doesn't update or trigger re-renders if they are referentially equal (===), so we don't need to avoid setting if they are equal. I believe that removes the need for state in the dependency array, which I believe removes the need for the first part of the if statement (not sure, though; again, I'm not sure of the purpose for that part) to avoid circular re-renders. We can trust this.getWebViewState will give us the appropriate value in any situation. As such, I think this rework would solve the problems here. I believe it also achieves keeping the state up-to-date when the key changes, which I do not believe will happen currently.

I will probably make some adjustments that include this in my PR FYI.

Again, this is just informational. No action needed unless you particularly desire to make the change yourself!

Thanks for these comments, super cool! I'd like to make the changes, but since this branch is already merged and deleted it might be easier if you just do it at part of your work, as you propose