Merge pull request #1133 from centerofci/1109_follow_up

Add code comments linking to discussions

mathesar_ui/src/App.svelte

@@ -9,11 +9,12 @@
   import { currentSchemaId } from '@mathesar/stores/schemas';
   import { beginUpdatingUrlWhenSchemaChanges } from './utils/routing';
 
-  // This is a bit of a hack to deal with our routing still being a patchwork of
-  // declarative and imperative logic. Without this call, the URL will not
-  // reliably set the query params when the schema changes. It actually _will_
-  // set the query params _sometimes_, but we weren't able to figure out why the
-  // behavior is inconsistent.
+  // Why is this function called at such a high level, and not handled closer to
+  // the code point related to saving tab data or the code point related to
+  // switching schemas?
+  //
+  // Because we need to place this at a high level in order to avoid circular
+  // imports.
   beginUpdatingUrlWhenSchemaChanges(currentSchemaId);
 </script>
 

mathesar_ui/src/component-library/pagination/Pagination.svelte

@@ -33,8 +33,11 @@
   // TODO: @seancolsen says:
   // > Refactor `pageCount` to no longer be an exported prop. We're exporting it
   // > just so the parent component can access the calculation done within this
-  // > component. That's an unconventional flow of data. I'd rather do the
-  // > calculation in the parent and pass it down to this component.
+  // > component. That's an unconventional flow of data.
+  //
+  // See https://github.com/centerofci/mathesar/pull/1109#discussion_r818638950
+  // for further discussion. @pavish and @seancolsen settled on an approach
+  // using a utils function.
   export let pageCount = 0;
 
   // ARIA Label for component

mathesar_ui/src/utils/routing.ts

@@ -1,9 +1,25 @@
+/**
+ * @overview
+ *
+ * Our routing system is a mix of declarative logic (e.g. in App.svelte) and
+ * imperative logic (e.g. `saveTabData` within `tabDataSaver.ts`). We need the
+ * imperative logic because the query param which saves the tab data is quite
+ * complex and would be difficult to implement declaratively.
+ *
+ * This file provides helpers for the imperative logic.
+ */
+
 import type { Unsubscriber, Writable } from 'svelte/store';
 import { get } from 'svelte/store';
 import { currentDBName } from '@mathesar/stores/databases';
 import { getTabsForSchema } from '@mathesar/stores/tabs/manager';
 import { saveTabData } from '@mathesar/stores/tabs/tabDataSaver';
 
+/**
+ * See https://github.com/centerofci/mathesar/pull/1109#discussion_r816957769
+ * for more information on the origin of the approach used in this function and
+ * why we need it.
+ */
 export function beginUpdatingUrlWhenSchemaChanges(
   currentSchemaId: Writable<number | undefined>,
 ): Unsubscriber {