Store Rubric in IndexedDB #9840

thsparks · 2024-01-31T21:41:39Z

This change stores the rubric in the indexed db and loads the last-active rubric automatically when the page loads. Currently only one rubric is really stored at a time, but the schema can support multiple if we want to evolve this over time.

Indexed DB has two tables:

Rubrics - where we store the rubrics, with the key set to the rubric name
Metadata - internal settings we may want to store, currently just lastActiveRubricName, but I imagine we may want to use this later to preserve preferences like auto-save and auto-run.

A note on code reuse:
The indexed db code is borrowed from Kiosk. I considered moving some of it into a shared class, but noticed we already have IDBWrapper in BrowserUtils. Then I tried switching to that, but I ultimately needed more code to manage the error handling than I saved by re-using the wrapper code, so I moved away from that approach as well. (It also wasn't working cleanly with custom key fields.) I think the right long-term move here is to evolve the IDBWrapper class to work better for these scenarios, but I think it'd require quite a lot of testing outside of the teachertool, so I was hesitant to make that change now.

…happens in a centralized place that listens for rubric state changes. Also still need to save when a criteria is removed.

… have additional fields we'd want to store here, like autosave and autorun preferences.

thsparks · 2024-01-31T21:43:48Z

pxtblocks/code-validation/runValidatorPlanAsync.ts

@@ -1,11 +1,14 @@
 namespace pxt.blocks {
+


@srietkerk The changes in this file are responding to your comments in #9835. They're not related to the indexed db change, I actually forgot I'd made them in this branch, but I suppose here they are 😅

eanders-ms · 2024-01-31T21:56:06Z

teachertool/src/transforms/setRubricName.ts

+        ...teacherTool.rubric,
+        name,
+    };
+    dispatch(Actions.setRubric(newRubric));


May want to provide feedback here that the name was saved. Post a notification?

Hmm, a notification feels a little too heavy-weight here to me. Ideally, I think the user would just feel like the name is always saved once they type it, similarly to how criteria adding/removing works, no real need for a "saved" notification. Maybe I should just save the name whenever the text is changed and debounce it a little, rather than requiring them to hit enter and/or click away from the Input?

Probably fine as is for now. Let's see what kind of feedback we get.

I did end up adding a debounced input to handle this instead...but I can change it back if you want :)

(also considered just wrapping the onChange call in pxt.Util.debounce but I got worried it wouldn't play nicely with React re-rendering things and possibly resetting fields)

teachertool/src/transforms/tryLoadLastActiveRubricAsync.ts

eanders-ms · 2024-01-31T22:01:23Z

teachertool/src/components/ActiveRubricDisplay.tsx


    return (
        <div className="rubric-display">
-            <h3>{lf("Rubric")}</h3>
-            {teacherTool.selectedCriteria?.map(criteriaInstance => {
+            <Input


needs ariaLabel

eanders-ms · 2024-01-31T22:09:52Z

teachertool/src/services/indexedDbService.ts

+
+const db = new TeacherToolDb();
+
+let initializeAsync = async () => {


This code actually doesn't work as intended. Due to the async, the function can be called multiple times before the reassignment happens. This is my bug :). I introduced it in Kiosk. One fix would be to have a global promise that does the db init and then resolves. getLastActiveRubricAsync, etc. would wait on that.

You could create a global promise that is the only source of the db instance. API calls have to await it to get the db instance. This would ensure no calls can be done on db before it's initialized.

eanders-ms · 2024-01-31T22:10:20Z

teachertool/src/services/indexedDbService.ts

+}
+
+export async function saveRubricAsync(rubric: Rubric) {
+    await db.saveRubric(rubric);


Need to await the db init here.

teachertool/src/state/reducer.ts

eanders-ms · 2024-01-31T22:27:57Z

teachertool/src/components/ActiveRubricDisplay.tsx

+                placeholder={lf("Rubric Name")}
+                initialValue={inProgressName}
+                onEnterKey={handleConfirmName}
+                onBlur={handleConfirmName}


Normally you would need to include the preserveValueOnBlur={true} prop here, but you might be getting around it by setting initialValue={inProgressName}

Ah, yeah. I think that was masking the issue. I'll still add it.

…ks/teacher_tool/store_rubric_in_indexedDb

… happens exactly once and all code that references the db must go through the promise to get it.

Co-authored-by: Eric Anderson <[email protected]>

…ps://github.com/microsoft/pxt into thsparks/teacher_tool/store_rubric_in_indexedDb

eanders-ms

Looks great.

…quiring the user to click away or click enter

eanders-ms · 2024-02-01T03:52:12Z

teachertool/src/components/DebouncedInput.tsx

+            clearTimeout(timerId.current);
+        }
+
+        timerId.current = setTimeout(() => {


What is the expected behavior if the component unmounts while a timeout is pending?

Hmm good question. I'll add some code to fire the onChange immediately if we're umounting and the timeout is pending.

eanders-ms · 2024-02-01T04:08:03Z

teachertool/src/components/DebouncedInput.tsx

+import { Input, InputProps } from "react-common/components/controls/Input";
+
+export interface DebouncedInputProps extends InputProps {
+    intervalMs?: number | undefined;


Consider assigning the default value here so it's less buried and available to intellisense.

intervalMs = 500

I don't think I can set it here, since this is an interface, but I've moved it up to the DebouncedInput parameter definition and added a comment here instead.

…the component, move default interval value for better visibility)

thsparks added 9 commits January 30, 2024 16:03

Validator service improvements

6a63633

Untested code to write / read rubrics from indexeddb

eb39f55

Create Rubric object, which also contains a field for name

ac06f95

Some saving and loading from indexeddb. May want to rework so saving …

f07434f

…happens in a centralized place that listens for rubric state changes. Also still need to save when a criteria is removed.

Save rubric when removing criteria

db4440d

Validate criteria when loading

3d531a4

Move save code into transform called by reducer

5068f1c

Update schema for indexeddb to separate metadata. I anticipate we may…

1dfc266

… have additional fields we'd want to store here, like autosave and autorun preferences.

Prettier

8d0e79c

thsparks requested a review from a team January 31, 2024 21:41

thsparks commented Jan 31, 2024

View reviewed changes

eanders-ms reviewed Jan 31, 2024

View reviewed changes

teachertool/src/transforms/tryLoadLastActiveRubricAsync.ts Outdated Show resolved Hide resolved

eanders-ms reviewed Jan 31, 2024

View reviewed changes

teachertool/src/state/reducer.ts Outdated Show resolved Hide resolved

eanders-ms reviewed Jan 31, 2024

View reviewed changes

thsparks added 3 commits January 31, 2024 14:59

Merge branch 'master' of https://github.com/microsoft/pxt into thspar…

de5d7b5

…ks/teacher_tool/store_rubric_in_indexedDb

Aria label and preserveValueOnBlur for rubric name input

05267c0

Use a global promise that initializes the db to ensure initialization…

0619155

… happens exactly once and all code that references the db must go through the promise to get it.

This comment was marked as outdated.

Sign in to view

thsparks and others added 2 commits January 31, 2024 15:18

Apply suggestions from code review

d3a95cb

Co-authored-by: Eric Anderson <[email protected]>

Merge branch 'thsparks/teacher_tool/store_rubric_in_indexedDb' of htt…

8f86a03

…ps://github.com/microsoft/pxt into thsparks/teacher_tool/store_rubric_in_indexedDb

eanders-ms approved these changes Jan 31, 2024

View reviewed changes

Create & use a debounced input to save the rubric name rather than re…

cc03024

…quiring the user to click away or click enter

eanders-ms reviewed Feb 1, 2024

View reviewed changes

DebouncedInput updates from PR (send event if pending and unmounting …

3f6314c

…the component, move default interval value for better visibility)

thsparks merged commit 1b92ccc into master Feb 1, 2024
6 checks passed

thsparks deleted the thsparks/teacher_tool/store_rubric_in_indexedDb branch February 1, 2024 18:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store Rubric in IndexedDB #9840

Store Rubric in IndexedDB #9840

thsparks commented Jan 31, 2024

thsparks Jan 31, 2024

eanders-ms Jan 31, 2024

thsparks Jan 31, 2024

eanders-ms Jan 31, 2024

thsparks Feb 1, 2024

thsparks Feb 1, 2024

eanders-ms Jan 31, 2024

eanders-ms Jan 31, 2024

eanders-ms Jan 31, 2024

eanders-ms Jan 31, 2024

eanders-ms Jan 31, 2024

thsparks Jan 31, 2024

This comment was marked as outdated.

eanders-ms left a comment

eanders-ms Feb 1, 2024

thsparks Feb 1, 2024

eanders-ms Feb 1, 2024

thsparks Feb 1, 2024


		const db = new TeacherToolDb();

		let initializeAsync = async () => {

Store Rubric in IndexedDB #9840

Store Rubric in IndexedDB #9840

Conversation

thsparks commented Jan 31, 2024

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

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

This comment was marked as outdated.

eanders-ms left a 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