Attempts/Questions klonen #240
Conversation
| function useHighlightOnInsert<T>( | ||
| items: Ref<Record<string, T>>, | ||
| idGenerator: (id: string) => string, | ||
| options: { | ||
| historyStateKey?: string | ||
| scrollBehavior?: ScrollIntoViewOptions | ||
| highlightDelay?: 600 | ||
| } = {}, | ||
| ) { | ||
| // Default options | ||
| const { | ||
| historyStateKey = 'highlightId', | ||
| scrollBehavior = { | ||
| behavior: 'smooth', | ||
| block: 'center', | ||
| }, | ||
| } = options | ||
|
|
||
| const pendingIds = ref<Set<string>>(new Set()) | ||
| const highlightedIds = ref<Set<string>>(new Set()) | ||
|
|
||
| // Handle highlighting after navigation | ||
| onMounted(() => { | ||
| const highlightId = history.state?.[historyStateKey] | ||
| if (typeof highlightId === 'string') { | ||
| pendingIds.value.add(highlightId) | ||
| } | ||
| }) | ||
|
|
||
| // Watch for pending items to appear in the list | ||
| watch(items, async (newItems, oldItems) => { | ||
| for (const id of pendingIds.value) { | ||
| if (newItems[id] && !oldItems?.[id]) { | ||
| pendingIds.value.delete(id) | ||
| await nextTick() | ||
|
|
||
| const el = document.getElementById(idGenerator(id)) | ||
| if (el) { | ||
| el.scrollIntoView(scrollBehavior) | ||
| // Unfortunately, scrollIntoView does not return a Promise | ||
| setTimeout(() => { | ||
| highlightedIds.value.add(id) | ||
| el.addEventListener( | ||
| 'animationend', | ||
| () => { | ||
| highlightedIds.value.delete(id) | ||
| }, | ||
| { once: true }, | ||
| ) | ||
| }, options.highlightDelay) | ||
| } | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| return { | ||
| handleNewItem: (id: string) => { | ||
| pendingIds.value.add(id) | ||
| }, | ||
| highlightedIds, | ||
| } | ||
| } |
There was a problem hiding this comment.
Ich verstehe nicht ganz die Dualität zwischen dem Watch auf items und dem Hinzufügen neuer Items zu pendingIds. (Das könnte aber auch einen Vue-Skill-Issue meinerseits sein.)
Damit etwas gehighlighted (highgelit?) wird, muss erst handleNewItem aufgerufen, und dann das neue Element zu items hinzugefügt werden, oder? Das wirkt relativ fragil bzgl. Race Conditions.
Und wir wollen doch eh immer das neue Element highlighten (oder nach Navigation das aus der History). Brauchen wir da überhaupt handleNewItem?
There was a problem hiding this comment.
Es kann erst zum neuen Eintrag gescrollt (und geheilei... hervorgehoben) werden, wenn die Daten gerefetched und gerendert sind.
Die aktuelle Implementierung nimmt zu behandelnde IDs auf zwei Arten an:
- über
handleNewItem - über einen konfigurierbaren key in
history.state, um die Mechanik auch nach dem Navigieren zu unterstützen.
pendingIds sind die IDs, die in der Zukunft noch behandelt werden müssen.
items ist ein (reaktives) ID-indiziertes Objekt von Items, die das composable überwacht und reagiert, sobald eines der pendingIds darin auftaucht. In unserem Fall kommen die items direkt aus dem Pinia Colada Store, also von der API.
und dann das neue Element zu items hinzugefügt werden, oder? Das wirkt relativ fragil bzgl. Race Conditions.
Diese Probleme sehe ich:
- dass wir mehrere Clones gleichzeitig haben, und die
scrollTosquasi miteinander kämpfen (passiert wenn man wild auf clone klickt). - aus irgendeinem Grund das neue Item in
itemsankommt, bevor noch das Composable das erste Mal läuft, also die Logik inwatchnie getriggert wird (könnte prinzipiell passieren, z.B. nach einer async Navigation).
Ich bereite gerade einen Patch vor, der
- auf einem globalen Store basiert, der "Operations" trackt.
- So kann ich global alle Buttons deaktivieren, so dass immer nur eine Clone-Operation zur selben Zeit läuft.
- Der State überlebt auch eine Navigation. Es muss kein State mehr in
history.statemehr versteckt werden. handleNewItemwird überflüssig.
- neue Item auch erkennt, wenn das Composable läuft nachdem das Item ankommt.
- das Composable wird dadurch etwas einfacher und hat weniger Verantwortlichkeiten.
There was a problem hiding this comment.
Das vorherige Composable useHighlightOnInsert habe ich in drei unabhängige Komponenten mit klaren Verantwortlichkeiten aufgeteilt.
pendingOperations store
Der neue Patch führt einen Store ein, der State für asynchrone Operationen zentral trackt.
- Damit wird das State-Management vereinheitlicht, insbesondere
- keine Parent-Child-Kommunikation über Events und
handleNewItemmehr, und - keine Kommunikation durch
history.state.
- keine Parent-Child-Kommunikation über Events und
- Es wird nur noch eine Operationen gleichzeitig erlaubt. (Unterbindet die gleichzeitige Ausführung mehrerer
scrollTos). - Während Operationen sind Aktions-Buttons und auch Page-Transitions deaktiviert, was verhindert, dass das UI in einen inkonistenten State gelangt, zB dass die Person von der Seite wegnavigiert bevor
deferred-itemkonsumiert wurde, etc.
useDeferredItem
- Überwacht eine Collection und reagiert wenn ein erwartetes Item erscheint.
- Konsumiert dafür ein
deferred-itemund ruft einen beliebigen Handler auf. - Im Gegensatz zu vorher is nun auch das andere Timing-Issue gelöst, nämlich, dass ein ein deferred Item übersehen wird, weil das Composable später ausgeführt wird als das Item tatsächlich ankommt.
useHintItem
Die ganze DOM-related scrollTo/Highlight-Logik.
- Nutzt zusätzlich einen Timer als Fallback, falls
animationendaus irgendeinem Grund nicht feuert, so dass das UI wieder freigegeben wird. - Verzichtet auf
window.getElementById, sondern setzt stattdessen auf eineref-Assigner-Funktion, um an die DOM-Elemente zu gelangen, was Vue-idiomatischer ist.
|
|
||
| await self._state_manager.write_attempt_score(question_id, attempt_id, attempt_scored) | ||
|
|
||
| async def clone_attempt(self, question_id: str, attempt_id: str, new_attempt_id: str) -> None: |
There was a problem hiding this comment.
Ich hätte hier tatsächlich einfach cp -r (bzw. shutil.copytree) gemacht. Spricht da etwas gegen?
There was a problem hiding this comment.
Du meinst in AttemptController.clone_attempt()? Dort ginge das im aktuellen Design gar nicht. Ich hatte die Controller und das state management mal decoupled irgendwann.
Die Controller reden nur mir dem StateManager-Protokoll, kennen gar keine Ordner oder Dateien. Dadurch könnten wir später mal das state management relativ einfach durch irgendwas anderes ersetzen, SQLite oder so.
Ich glaube, die eigenliche Schwäche am aktuellen Design ist, dass der Webserver keinen Data Layer besitzt und diese Logik auf die Controllermethoden abwälzt. Da gibt's auf jeden Fall Refaktorisierungspotential, um die Controllermethoden schlanker und übersichtlicher zu machen.
Davon abgesehen würde cp -r nicht erkennen, ob ein Attempt überhaupt gültig ist, also zumindest valide data- und score-Dateien besitzt, und natürlich auch einen 626 GiB großen tarball mitkopieren, den jemand scherzeshalber oder aus Versehen im Attempt-Ordner lagert.
There was a problem hiding this comment.
Ja, ich meinte schon das Hinzufügen einer StateManager.clone_attempt() Methode. Bzgl. Data Layer: Bitte nicht noch eine Abstraktionsebene, die Trennung lohnt sich hier nicht.
Ich glaube nicht, dass uns über 100% PEBKAC-Fälle wie das Tarball-Beispiel sonderlich viele Gedanken machen sollten.
Also, refactor scrolling/highlighting behavior into `useHighlightOnInsert` composable. Closes: #238
pytest tries to collect `*.py` files in `frontend/node_modules`, which causes errors.
…n clone operations
Split `useHighlightOnInsert` into focused composables with well-defined responsibilities.
- Add `<BackLink>` component. - Implement `<LoadingIndicator>` component. - Switch to Pinia Colada's `isPending` (tracks first request) instead of `asyncStatus`. - Replace manual margins with Bootstrap `vstack` in main page and list views
|
Mit der Einführung von In 9d374cb habe ich schlussendlich noch eine |
Backend
Frontend
Issue Attempts/Questions klonen #238