CLUE-550 Stage 3b: Sort Work + workspace UI for class-wide documents - #2942
CLUE-550 Stage 3b: Sort Work + workspace UI for class-wide documents#2942scytacki wants to merge 41 commits into
Conversation
Describes what the Stage-3 PR delivers: scope guards resolving the deferred scope-modeling checkpoint, explicit-null scope fields making the class+unit scope queryable (plus a backfill pass on the renamed axes script), a unit-scoped Sort Work listener, Whole Class sectioning with structured sort keys, presentation driven by the concurrent/kind axes, one shared edit predicate for Sort Work and the resources pane, and an emulator test establishing history-write authorization for synthetic document owners. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Add pure predicates to read document scope from stored association fields without consulting the kind registry, enabling consumers like Sort Work to distinguish documents by scope regardless of whether their kind is registered in the current session. These guard functions pin the implementation against all stored document shapes: personal, problem, group, exemplar, class-wide, and legacy class-wide documents. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CLUE-550] Stamp investigation:null and problem:null on classUnit-scoped documents instead of omitting them, so Firestore can query for documents scoped to a unit but not a problem (a null-valued field is queryable; a missing one is not). Widens IDocumentScopeContext.investigation/.problem to accept null to match. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sses [CLUE-550] Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CLUE-550] byGroup now reads a document's stored scope (hasGroupScope/hasClassUnitScope) instead of branching on doc.type === GroupDocument, and files class-wide documents into their own "Whole Class" section. sortGroupSectionLabels is replaced by sortGroupSections, which orders sections from a per-label GroupSectionSortKey (class/group/none) carried alongside the display label, rather than parsing group numbers out of the (translatable) label text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…UE-550] Add kNoNameSectionLabel constant and update byName sorting to file class-wide collaborative documents in their own "No Name" section rather than under an unknown author. Update all affected tests to account for the new section's alphabetic placement between "Cytacki" and "Swenson". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… filters [CLUE-550] Adds a third Firestore listener to watchFirestoreMetaDataDocs that fetches documents scoped to the unit but not to a problem (class-wide collaborative documents) whenever the Investigation or Problem filter is applied, since the filtered query's investigation clause would otherwise exclude them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replace two disagreeing Edit-button checks (Sort Work's ownership-only check, the resources pane's my-work/learningLog tab check) with a single canUserEditDocument predicate: own document always editable, otherwise only a concurrent document from inside its scope (class-wide by class membership, group by group membership). Fields are read per-field from the reactive Firestore metadata, falling back to the document, so the Edit button appears as soon as a groupmate's document metadata syncs. Behavior change: a bookmarked document owned by another student no longer shows Edit in the My Work tab, since the old tab check allowed it regardless of ownership or scope. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ment [CLUE-550] canUserEditDocument granted Edit on a user's own published documents, since a publication's uid is the publisher's — the ownership arm alone couldn't distinguish a live document from its read-only published copy. It also granted researchers Edit on class-wide documents whenever their observing classHash matched, though researchers get no write affordance elsewhere in the app. Check isPublishedType before the ownership arm and deny researchers explicitly. Teachers keep editing class-wide documents in their own class, now pinned by a dedicated test. Also cover the document-only call path (no metadata) used by the resources pane, and a case where an empty context_id would otherwise match a user's default empty classHash if the `!!contextId` guard were removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CLUE-550] Characterization tests against a Firestore emulator confirmed history create/read on a concurrent document owned by a synthetic group_ or class_ uid was denied by the pre-existing rule, which gates on userOwnsDocument() resolved through the parent document's real uid. Rebases the history rule onto the concurrent axis: create and read are now also allowed when the parent document carries concurrent: true and the requester's class_hash matches its context_id, matching the existing RTDB write grant on the whole classes/<classHash> subtree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…thorization [CLUE-550] concurrent is now an authorization input for the concurrent-document history rule (isConcurrentClassDocument), but nothing kept it read-only: any class member could update a classmate's ordinary document with { concurrent: true }, which was allowed (no read-only field touched, class members can update any in-class document) and forged that classmate's history access for the whole class. type is itself read-only, so it can't be flipped first to route around a type check placed elsewhere. Closes the escalation at the write path: a new concurrentChangeOk() allows a change to concurrent only when the stored document's type is "group", wired into isValidDocumentUpdateRequest(). This stays transitional — two paths still merge-update concurrent onto pre-existing group documents that predate the field (the on-open backfill in src/lib/db.ts, and the one-shot scripts/backfill-group-document-axes.ts) — so concurrent can't yet be made unconditionally read-only. Once both backfills have run everywhere, concurrentChangeOk() should be deleted and concurrent folded into preservesReadOnlyDocumentFields's read-only set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Update the document-axes roadmap's kind/scope/behavior-modules rows and Current-effort paragraph for what Stage 3 delivered, and record the eager-open cost measurement (a real ~700ms fast-path delta against demo/units/qa, obtained via a live Chrome session and Firestore project) and the manual end-to-end check as a pending human-verification checklist. Also correct the concurrentChangeOk transitional-rule breadcrumbs (in firestore.rules, the backfill script, and both stage design specs): once concurrent becomes settable only at document creation, the create path also needs constraining, since isValidDocumentCreateRequest today constrains neither concurrent nor uid. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cs, narrow hasGroupScope [CLUE-550] getDocumentTitle mislabeled a cross-unit class-wide document (type:"group", unregistered kind, no groupId) as "Group undefined Document" by falling through to the group-title branch on type alone; it now also requires groupId. document-scope.md, the Stage-3 design doc, and document-scope.test.ts claimed exemplars carry no unit — they carry both a unit and an investigation, and it's the investigation that excludes them from hasClassUnitScope; docs and the test fixture now reflect that. hasGroupScope is now a type predicate, removing a cast and a dead fallback in document-group.ts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Under the Sort Work "All" filter a class sees documents from every unit it has worked through, and only the current unit's config is loaded. A class-wide document from another unit therefore had no resolvable title and stores none, so it rendered blank; and where two units declare the same kind, the current unit's authored wording was applied to a document it does not govern. Record the declaring unit on the kind registration and return a registered title only for that unit's documents. A document that resolves no title and stores none is named by its kind plus its curriculum scope, read from the stored fields so it holds for any scope shape. getDocumentTitleFromProblem shares the scope label, so the "sas-1.2" format has one definition. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A kind declared in configuration is only defined while that configuration is loaded, and kind names are not unique across configurations. So a document must stay interpretable without its kind's definition, and must carry the association naming the configuration that defined it. For unit-declared kinds that association is `unit`, which bounds them to unit-scoped documents or narrower. Making personal-like presets authorable needs a configuration source loaded independently of the current unit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
getDocumentTitle selects the group-document title on `type` plus a groupId because a group document may carry no stored kind, and the lists showing these titles render before a document is opened to backfill one. Once the backfill has stamped kind on every group document the check becomes `kind == "group"`, and the groupId term goes with it: a class-wide document has its own kind and can no longer reach that branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Scope is two linear nestings, not one ordered level: curriculum (unit -> investigation -> problem) and owner (class -> group -> user). Every document sits somewhere on both, which is why no scopeLevel enum fits. Name each guard for its dimension — hasGroupOwnerScope and hasUnitCurriculumScope — and have it read only that dimension's fields. The curriculum guard drops its !groupId term, since a group document is already excluded by being narrowed to an investigation, and gains !offeringId: an offering assigns one problem, so it narrows that same dimension. An offering belongs to neither hierarchy. It is the assignment of a problem to a class — a point in their product — so it fixes curriculum scope while leaving owner scope free, which is how one offering holds both user-owned and group-owned documents. The class association is likewise not an owner level: every document names a class, and naming one is not being owned by one. The model is written once, in docs/document-scope.md; the axes doc, the design spec, and the module header point there rather than restating it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Requirements stated independently of how the owner is stored, so a change of representation can be checked against them. Each names where it is exercised. The Sort Work requirement is spelled out, since it constrains the representation most: both getters branch three ways on owner type, each branch resolves something different, and one of them emits a document once per group member. The type has to be readable from the document before any id is resolved. Two assumptions did not survive contact with the code: no Firestore query filters documents by owner type, and none filters by user owner. Sorting by owner is a client-side projection over documents already fetched by class, so the representation does not have to be queryable by type today. Also records a live limitation — a group owner resolves only against the current offering, so a group document from an earlier one is dropped from the by-name listing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The edit gate read `documentMetadata?.groupId ?? document?.groupId`, but those two fields carry different meanings. On a document model, `groupId` is the author's current group, refreshed as groups change; only on the Firestore metadata is it the group that owns the document. The fallback could therefore answer an ownership question with an unrelated value. It also compared bare group ids. Groups live under an offering, so the same group number in another offering is a different set of students — and Sort Work's "All" filter lists documents from every offering the class has worked through, so those documents do reach the check. Compare owners instead. A group document's owner carries its offering, which makes the comparison exact and removes the ambiguous read entirely: the predicate no longer looks at `groupId` at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Scope was carrying two independent facts: where a document is kept, and what content it is about. Bundling them is the same defect the axes work exists to undo, so they become separate axes. `container` is a strict nesting — class, classUnit, offering — and a document sits at exactly one node. Neither the user nor the group is a level: a container has to outlast what it holds, and group membership changes within an assignment and differs between them. Whose a document is belongs to `owner`. `curriculum` is what the document is about: nothing, unit, investigation, problem. The two usually agree, and exemplars are where they come apart — an exemplar is about one problem but belongs to no assignment, since it exists whether or not the class was ever assigned that problem. Also records that a canonical slot is a container plus an owner plus a label, and that class audiences are named by the container while group audiences come from the owner. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…LUE-550] The list read as a tour of the code. Rewrite it so each entry says what CLUE has to be able to do, and mark the exceptions rather than blurring them: a few entries record a constraint the current implementation imposes, and each says what it would take to lift. Moves the edit gate into Authorizing, where it belongs, and states the requirement it was missing — the check has to be specific to one offering, because group numbers repeat across them. Adds a Broken behavior section: a group owner resolves only against the current offering, so a group document from an earlier one is dropped from the by-name listing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
document-scope.md is about a different meaning of scope: how code at the tile level reaches things at the document level, surveyed as tree traversal, MST environment, React context, tile props, and the tile API. Sections about where a document sits on the axes were filed there because the word matched, not the meaning, leaving a reader looking for either topic to find the other. Restores that file to what it was, and moves the axes material to a new current-state doc alongside axes.md and target-architecture.md: the guards that exist, the fields behind them, what each stored shape looks like, and what has no guard yet. Records that the helper names still say "scope" from before that axis was split. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…riculum [CLUE-550] The axes docs describe container (where a document is kept: class → classUnit → offering) and curriculum (what it is about) as separate axes, while the code still called both "scope" and asked one about the other. Creation side: - A kind now declares `containerType` ("class" | "classUnit" | "offering") beside its existing `ownerType`. There is no group container level: a group document is kept in the offering alongside the problem documents its members write, and what makes it the group's is its owner. - `getDocumentOwnerFields` stamps a group owner's `groupId`, keyed on `ownerType`. It was previously stamped by a `group` scope type, which stated owner data on the wrong axis. - `getDocumentScopeFields` becomes `getDocumentLocationFields`, named for the pair of axes whose fields it returns rather than for the container alone. Read side, in the renamed `document-axes.ts`: - `hasGroupOwner` and the new `hasClassOwner`, which hides the fact that a class owner is a synthetic `class_<classHash>` uid with no field of its own. Sort Work sections a class-wide document under the class because of who owns it, so it now asks this rather than testing the curriculum. - `isInClassUnitContainer` replaces the curriculum test in `canUserEditDocument`. A classmate may edit a class-wide document because of where it is kept, which is a container question. - `hasUnitCurriculum` is gone; nothing asks a yes/no curriculum question now, and leaving it invited the same axis confusion back. `isInClassUnitContainer` needs `offeringId`, the only positive marker of the offering container — an exemplar carries the same unit/investigation/problem as a problem document. It is written to Firestore but was declared on no type, so nothing could read it; it is now on `IDocumentMetadataBase`, `DocumentMetadataModel`, and `DocumentModel`. Without it a group document would read as class-wide and any classmate could edit it, so the edit-gate fixtures now carry the offering and class a real group document has. An exemplar belongs to no offering either, so it shares that container and the gate's `concurrent` check is what keeps it read-only. That holds for both of its shapes, and each is now pinned: the curriculum document, and the metadata record a teacher's comment creates — the latter stamped with the commenting class's `context_id`, so nothing else would stop it. Also renames `GroupSectionSortKey.scope` to `.section`; it categorizes Sort Work sections and never meant the axis. The stored metadata is unchanged: a group document still gets the same `groupId`, now from the owner axis rather than the scope axis. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…six [CLUE-550] The gate read each field as `documentMetadata?.x ?? document?.x`, which implied the two sources could disagree and that the fresher one should win per field. They cannot: every field it reads — `uid`, `type`, `concurrent`, `unit`, `offeringId`, `context_id` — is stamped once at creation. The fallback exists for a simpler reason, that one of the two call sites supplies no metadata at all: the workspace opens documents without looking their metadata up, while Sort Work passes both. So choose the source outright, as `isDocumentAccessibleToUser` directly above already does, and hand it to `isInClassUnitContainer` whole rather than picking fields out at the call site. This also removes an unintended fallthrough. `DocumentMetadataModel` fields are `maybeNull` and `??` falls through on null, so a class-contained document — which stores `unit: null` explicitly — was reading its unit off the document instead. The answer was the same, but the sources were being mixed in a case nobody chose. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
] A canonical slot is a container plus an owner plus a label, but the pointer path expressed the owner only for group documents, and expressed it as `groups/<groupId>`: canonical/v1/classes/<h>/(offerings/<off>|units/<u>)/[groups/<grp>]/slots/<label> Every slot now carries an owner segment holding the document's own `uid`, synthetic owners included: canonical/v1/classes/<h>/[offerings/<off>|units/<u>]/owners/<uid>/slots/<label> Taking the owner from `uid` is what makes this a simplification rather than another branch. The rules already have `data.uid`, so `canonicalPointerPath` drops its `groupId` conditional and `data.groupId` disappears from firestore.rules entirely — no group id to read, no owner type to infer. The two create blocks now cover every owner type, so they scale with container levels rather than container x owner, and the anticipated user-owned slot for problem/planning documents needs no new segment form. The class still leads the path: the read rule compares that segment to the caller's class claim, which a synthetic group owner id cannot supply. Below it each container names only itself — an offering contributes its own id, not the classUnit it falls inside. Pointers are immutable by rule and so cannot be moved; addressing each by its own container is what would let a container level be inserted later without stranding the pointers that already exist. Callers now pass the container and let getOrCreateCanonicalDocument derive the owner from the kind, through the same getDocumentOwner call that stamps the document's `uid`. Previously the caller assembled the path's owner while the registry derived the document's; with the owner in the path, a divergence would surface as a rules rejection instead of being caught here. getDocumentOwner now throws for an unregistered kind instead of defaulting to the creating user. Defaulting would hand a group's or a class's document to whoever created it and file it in that user's slot, both silently. It also confines document creation to the kinds the current unit declares, since a unit-declared kind is registered only while its unit is loaded. Two db tests were relying on that default, calling getOrCreateClassWideDocument without the registration production performs first; they now register the kind. Existing group-document pointers self-heal: the new path misses, findLegacyGroupDocument adopts the document, and the backfill writes a pointer at the new path. Class-wide documents have no legacy fallback, so a dev class holding one gets a second; they are unreleased, so no production data is affected. Pointers at the old paths become unreachable orphans. Also renames the rules' `hasScopeField` to `hasPresentField`, which is what it tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CLUE-550] A group id is unique only within an offering: group 3 of one assignment and group 3 of the next are different sets of students. Sort Work's "All" filter spans every offering the class has worked through, and its by-name sort looked the group up with `getGroupById(doc.groupId)` against a store that holds only the current offering's groups. So a group document from an earlier assignment was listed under whoever is in this offering's group 3 now. Worse, when this offering had no group of that number, the lookup returned undefined, `group?.users.forEach` short-circuited, and nothing filed the document at all — it vanished from the sort. The groups store now records which offering it holds, and `getGroupByOwnerId` matches a document's whole owner uid, building each candidate with `getGroupOwnerId` rather than taking a stored id apart. A group outside that offering resolves to nothing, which is the truth: its membership is not knowable from here. Those documents get one section per assignment rather than per group — a class works through many, and a section per group of each would swamp the sort. Every document in an offering shares a curriculum position, so `getCurriculumLabel` names the section: "Groups from sas-2.4". The by-name sort gained sort keys, like the by-group sort already had, so these sections and the authorless one order after the named students instead of landing alphabetically among the surnames. That moves "No Name" to the end, matching the by-group sort's trailing "No Group"; three tests documented the old position in their comments. Group membership often does carry over between assignments, which is why the wrong attribution usually looked right. If showing a previous assignment's groups by name is wanted, it needs that offering's membership loaded deliberately. `getGroupOwnerId` also gives the synthetic group-owner grammar one home, replacing the template literal in `userIdForGroupDocuments`. That getter now returns undefined when the user is in no group instead of building an id containing "undefined", and `getDocumentOwner` refuses to create a group- or class-owned document without its synthetic owner rather than falling back to the creating user — which would have made one student the owner and filed the document's canonical slot under them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…er [CLUE-550] `hasGroupOwner` tested the stored `groupId`; it now tests the `group_` prefix of the owner uid, the same way `hasClassOwner` tests `class_`. Each prefix is a constant shared by the function that mints the uid and the guard that reads it back, so both owner guards read one field and the uid is the single authority on who a document belongs to — it is already what the canonical slot is addressed by and what the Firestore rules read. Testing a prefix is not the same as taking a uid apart: nothing recovers an `offeringId` or a `groupId` out of one, which is why `getGroupByOwnerId` builds the whole owner id and compares. `groupId` leaves `IDocumentAxisFields`, since no guard reads it. Sort Work's `byGroup` used the guard's type narrowing for its section label and now asks for the label explicitly; `byName` already went through `getGroupByOwnerId(doc.uid)` and is unchanged. Behavior is unchanged on stored data. A `groupId` with a non-`group_` uid has never been written — only group documents have ever had the field stamped — and a `group_` uid without a `groupId` does not exist, the two having been introduced together. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CLUE-550] `canUserEditDocument` answers a permissions question by composing the other axes by hand. Point at it from the axes docs as a legible example of that, and note in the docblock which axis it belongs to. Permission decisions are spread widely through the code, so the example is called out for being compact enough to read whole, not for coming first. Two consequences are recorded where it lives: its `type` test for published documents is the last type branch inside the gate, and its `concurrent` test would likely dissolve too, since a policy granting write to a class or group states the multi-writer case directly. What is left after that is not permission logic but resolving which class or group the grant points at, which the owner and container guards already answer. Docs only — no behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… key [CLUE-550] The three `as GroupSectionSortKey` casts existed only because the object literals had no contextual type, so `section` widened to `string` and would not satisfy the union. Naming the label-and-sort-key pair and annotating the two functions that build one supplies that context, and `section` stays a literal. This is the mechanism `byName` already relied on: its `addDocToSection` parameters give its literals a contextual type, which is why it needed no casts. `byGroup` returns the pair rather than passing it, so the type had to be named. The casts were also the weaker kind — `as` would have accepted a wrong `section` value silently, where a typo is now a compile error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
collaborative-learning
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Project |
collaborative-learning
|
| Branch Review |
CLUE-550-stage-3b-axes-sortwork
|
| Run status |
|
| Run duration | 12m 48s |
| Commit |
|
| Committer | Scott Cytacki |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
4
|
|
|
2
|
|
|
5
|
|
|
0
|
|
|
216
|
| View all changes introduced in this branch ↗︎ | |
Tests for review

document_tests/tiles_copy_test_spec.js • 1 failed test • Regression tests
| Test | Artifacts | |
|---|---|---|
| Test copy tiles from one document to other document > Verifies copy button states based on tile selection |
Test Replay
Screenshots
|
|

teacher_tests/teacher_sort_work_view_1_spec.js • 1 failed test • Regression tests
| Test | Artifacts | |
|---|---|---|
| SortWorkView Tests > should open SortWorkView tab and interact with it |
Test Replay
Screenshots
|
|

teacher_tests/teacher_sort_work_view_3_spec.js • 1 failed test • Regression tests
| Test | Artifacts | |
|---|---|---|
| SortWorkView Tests > should open Sort Work tab and test secondary sort functionality |
Test Replay
Screenshots
|
|

teacher_tests/teacher_sort_work_view_6_spec.js • 1 failed test • Regression tests
| Test | Artifacts | |
|---|---|---|
| SortWorkView Tests > should retain selection state when going between sort options and viewing a sorted document |
Test Replay
Screenshots
|
|

document_tests/group_test_spec.js • 1 flaky test • Regression tests
| Test | Artifacts | |
|---|---|---|
| Test group functionalities > 4-up view tests |
Test Replay
Screenshots
|
|

teacher_tests/history_playback_spec.js • 1 flaky test • Regression tests
| Test | Artifacts | |
|---|---|---|
| History Playback > verify playback icon not displayed for student |
Test Replay
Screenshots
|
|
…e-3b-axes-sortwork # Conflicts: # docs/document-metadata/metadata-fields.md # src/models/document/document-utils.test.ts # src/models/document/document-utils.ts
Stage 3 grew past 47 changed files and was split into two stacked PRs, so this spec now covers both. The note at the top maps its sections onto them: the presentation work landed in the precursor, everything else here. It also names the two changes the precursor carries that this spec never described — surfacing `offeringId` and splitting `authorGroupId` off `groupId`, both found during implementation — and flags the two statements the first of those overtook. Those are left as written rather than corrected: this is a record of the design as it stood, and editing it would erase the evidence that surfacing `offeringId` was a discovery rather than part of the plan.
…e-3b-axes-sortwork # Conflicts: # docs/document-metadata/metadata-fields.md
|
If the stage 2 PR makes it to production before this 3b PR, we should consider keeping old class-wide documents working. That would probably mean bumping the version string in the canonical pointer paths and updating the code to handle both versions. Hopefully we won't have to do that. |
|
Consider switching default group doc canonical label to just being its kind. We'll probably need each document to have a different title at least and doing that currently means giving it a new kind. If we changed the title approach so it wasn't based on the kind, which has other benefits, then it would be reasonable to keep the slot label separate from the kind. |
…e-3b-axes-sortwork Stage 3a's review-feedback pass landed on the same code this branch reworked, so each conflict keeps 3a's intent expressed in this branch's vocabulary: - `registerClassWideDocumentKind` (3a moved the class-wide kind shape into document-kinds.ts so every kind shape is declared beside the built-in ones) is kept, and takes the declaring unit this branch stamps so getDocumentTitle can tell two units' documents of the same kind apart. db.ts calls the helper again instead of inlining a registerDocumentKind call. - `requireGroupContext` is kept, and getOrCreateGroupDocument takes its narrowed `offeringId` for the container this branch passes in place of a prebuilt pointer path. - The kind registry keeps `containerType`; 3a's edit to that field's comment was dropping the "(see DocumentScopeType)" pointer, which is applied here. - `groupIdOfUserOwner`'s doc comment takes 3a's wording, plus this branch's pointer to the document-axes owner guards. - docs: the axes README keeps 3a's link to "Which documents get stamped" ahead of this branch's Stage 3 paragraph; metadata-fields keeps 3a's `groupId` wording plus this branch's note that `hasGroupOwner`, not this field, decides group ownership. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three statements in the owner axis's requirement list described an earlier design rather than the code beside them: - Requirement 6 had the client and the rules rebuilding a canonical-pointer path from the offering and the group as separate segments. The path carries the owner as one segment instead — the document's `uid` verbatim, synthetic owners included — with the offering coming from the document's container and no segment naming the group. What is left of the requirement is that an owner serialize to a single Firestore path segment both sides can use. - Requirement 8 bundled a group's members together with its "Group N" label. Only the members go through the offering-scoped group registry; the label is taken from the document's stored group id, so it needs nothing resolved and works for a document from any offering. - "Broken behavior" said sectioning by name drops a group document from another offering. It files the document under "Groups from <problem>" rather than dropping it, so the section is retitled "Limitations" and now states what actually remains: such a document's members are unknowable outside the current offering, and lifting that needs a group registry spanning offerings rather than a change to how the owner is stored. The limitation entry also records the deliberate asymmetry it exposes: sectioning by group is unaffected, because a group number is a label that reads the same in every offering while a student's name asserts who did the work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bound on which documents a unit-declared kind can create was written in terms of where a document is kept. It belongs on what the document is about. Rule 2 requires the document to carry a `unit`, and the curriculum axis is a nesting rooted at the unit — nothing, unit, investigation, problem — so "about that unit or narrower" is exactly "carries a `unit`". The bound becomes a restatement of the rule rather than something derived from another axis. The container levels imply a unit only incidentally, because `containerType` currently fixes both axes' values. That coupling is what made the container phrasing look right, and it is load-bearing in a way the phrasing hid: a kind declaring a class container and a unit curriculum would stamp `unit` and satisfy rule 2, yet a container-based bound forbids it. Kinds are expected to declare curriculum values eventually, so the curriculum phrasing is the one that stays correct. A second note records the mechanism behind that coupling, so it is visible when it changes: `containerType` is the only location value a kind declares, and `class` yields no unit, `classUnit` the unit, `offering` the problem. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…LUE-550] Four comments each re-argued that a title authored in one unit's config names only that unit's documents, so a document from another unit must not borrow it. The reasoning belongs to `IDocumentKindInfo.unit`, which exists for it; the other sites now state their own job and point there. - getDocumentTitle: keeps the rule and the fallback, drops the re-derivation it gave immediately after naming the field it came from - registerClassWideDocumentKind: drops the aside about what the unit prevents - createDeclaredClassWideDocuments: keeps the local coupling worth knowing at the call site — the unit passed is the code stamped as the document's `unit` - getUnresolvedDocumentTitle: drops the enumeration of the no-title cases and merges the two paragraphs Comments only; no behavior change. Also drops an unused `IDocumentMetadata` import from document-utils.test.ts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
] Two comments described `DocumentModel.groupId` as the author's *current* group on a problem document, refreshed as membership changes. That was true while the field was filled from `groups.groupIdForUser`; it now mirrors the stored owner-axis field and is set only on group documents, with the author's group living on `groupIdOfUserOwner`. - isUserInDocumentsGroup: the paragraph is removed rather than corrected. Why the comparison is on owners and not group ids is already answered above it — a group id is unique only within an offering — so a rewrite would say the same thing twice. - The canUserEditDocument test kept the stale premise as its justification. The test still earns its place, so only the comment changes, to the invariant it actually pins: a `groupId` on the model does not make a document group-owned; the owner uid does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR advances CLUE’s class-wide collaborative documents work (CLUE-550 Stage 3b) by formalizing “axes” (owner/container/curriculum) as field-based guards, updating Sort Work to properly surface and section class-wide documents, unifying edit gating, and migrating canonical pointer addressing/rules to a container+owner+label model (with supporting backfill tooling and rule tests).
Changes:
- Introduces document-axis guards and updates document kind registry/creation to stamp owner + location fields consistently (incl. explicit-null curriculum fields for unit-scoped docs).
- Updates Sort Work grouping/sorting/listening to properly include class-wide documents under Investigation/Problem filters and to section by owner (with sort keys instead of parsing labels).
- Migrates canonical pointer paths and Firestore rules/tests to use container + owner + label; adds/updates backfill script and strengthens history authorization for synthetic owners.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utilities/sort-document-utils.ts | Adds section label constants, sort-key types, and new section-sorting helpers (group/name). |
| src/utilities/sort-document-utils.test.ts | Adds unit tests for new group-section ordering behavior. |
| src/models/stores/user.ts | Uses shared getGroupOwnerId() and makes userIdForGroupDocuments return undefined when not applicable. |
| src/models/stores/sorted-documents.ts | Adds whole-unit Firestore listener/map to keep unit-level docs visible under narrower filters, with dedupe. |
| src/models/stores/sorted-documents.test.ts | Adds tests for the whole-unit listener lifecycle, dedupe, and clearing behavior. |
| src/models/stores/groups.ts | Adds offeringId tracking and getGroupByOwnerId() to resolve group documents safely across offerings. |
| src/models/stores/groups.test.ts | Tests new offering-aware owner-id resolution and offeringId recording. |
| src/models/stores/document-group.ts | Reworks Sort Work sectioning by owner axes; uses structured sort keys and handles cross-offering group docs. |
| src/models/stores/document-group.test.ts | Updates/expands tests for new sectioning rules (Whole Class / No Name / cross-offering). |
| src/models/document/document.ts | Ensures metadata includes context_id for document-only permission/title paths. |
| src/models/document/document-utils.ts | Unifies edit gating in canUserEditDocument(); improves fallback titling for unregistered kinds. |
| src/models/document/document-utils.test.ts | Adds extensive tests for class-wide/cross-unit titles and the new edit predicate contract. |
| src/models/document/document-kinds.ts | Renames scope→container concepts, adds unit-scoped titles, owner/location stamping helpers, and registry-free kind labels. |
| src/models/document/document-kinds.test.ts | Updates tests for owner/location behavior, unit-scoped titles, kind labels, and stricter owner resolution. |
| src/models/document/document-axes.ts | New: field-only guards for owner/container axes + curriculum labeling utilities. |
| src/models/document/document-axes.test.ts | New: tests pinning guards/labeling across stored document shapes. |
| src/lib/scoped-document-pointers.ts | Changes pointer addressing to container + owner + label (ICanonicalSlot) and updates path builder. |
| src/lib/scoped-document-pointers.test.ts | Updates/extends tests for owner-segmented pointer paths and container omission for class-level slots. |
| src/lib/db.ts | Updates canonical get-or-create to derive pointer path owner from kind; stamps owner/location fields; registers unit-declared kinds with unit context. |
| src/lib/db.test.ts | Updates tests for class-wide document stamping and owner-resolution assumptions. |
| src/lib/db-listeners/db-groups-listener.ts | Passes offeringId through to groups.updateFromDB() for offering-aware resolution. |
| src/components/navigation/document-view.tsx | Switches Edit button gating to shared canUserEditDocument(). |
| scripts/backfill-group-document-axes.ts | New: combined backfill for group-typed docs (concurrent/kind for group-owned; null curriculum for class-wide). |
| scripts/backfill-group-document-axes.test.ts | New: unit tests for dual-pass backfill selection, safety, and idempotence. |
| scripts/backfill-group-concurrent.ts | Removed: superseded by combined group-document-axes backfill script. |
| scripts/backfill-group-concurrent.test.ts | Removed: superseded by combined group-document-axes backfill tests. |
| firestore.rules | Updates canonical pointer rules to container+owner pathing; adds concurrent write guard and concurrent-history authorization. |
| firebase-test/src/documents-rules.test.ts | Adds rules regressions for concurrent forgery prevention and synthetic-owner history access. |
| firebase-test/src/canonical-pointers-rules.test.ts | Updates pointer paths for owner-segmented slots; expands integrity tests for owner isolation. |
| docs/superpowers/specs/2026-07-27-clue-550-stage-3-sort-work-ui-design.md | Adds design spec covering stage 3 behavior and rationale. |
| docs/document-metadata/metadata-fields.md | Updates metadata field docs for canonical slot semantics and offeringId/runtime notes. |
| docs/document-axes/README.md | Updates roadmap/status and adds link to “reading axes in code” doc. |
| docs/document-axes/reading-axes-in-code.md | New: documents current guard helpers, stored shapes, and known gaps. |
| docs/document-axes/axes.md | Expands/refines axes model (splitting container vs curriculum, etc.) and updates tables accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ups [CLUE-550] Two findings from review. **`concurrent` could be cleared, not just set.** `concurrentChangeOk` asked only that the document be `type == "group"`, so any class member could write `concurrent: false` onto a group document — stripping the whole group's shared history access and its members' Edit button. Not an escalation, which is what the rule was written against, but a denial available to anyone in the class. Both backfill paths write only `true`, so the rule now requires the resulting value to be `true` and clearing is denied. The regression test was checked for discrimination, not just for passing: against the previous rule it fails with "Expected request to fail, but it succeeded". `ISpecDocumentDoc.add` gains `boolean`, matching its sibling `ISpecHisoryDoc`, so a test can state a document's `concurrent` field at all. **`updateFromDB`'s `offeringId` was optional.** A caller omitting it would clear the stored offering while leaving the groups in place, and `getGroupByOwnerId` resolves nothing without one — so group-owned documents would quietly stop resolving to their members. Only one production caller exists and it always passes the value, so this is a latent trap rather than a live defect; the argument is now required. Required rather than only-assign-when-provided: these groups mean something only paired with the offering they came from, so a caller with no offering must clear the stale one instead of leaving a previous offering's id standing over a new set of groups — the same cross-offering confusion this branch exists to remove. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
This is replaced by #2949 |
CLUE-550 Stage 3b — Sort Work + workspace UI
Stage 3 of the class-wide collaborative documents work. Stage 2 created the Driving Question Board but showed nothing; this PR makes it visible, sectioned, titled, and editable.
Stage 3a (#2940) has merged, so this PR targets
masterand holds only the Stage 3b half. 3a was split out of this branch and carriedofferingIdonto both metadata types, the thumbnail and title bar readingconcurrent, and the split of a document's owning group (groupId) from its owning user's group (groupIdOfUserOwner). This PR's container guard builds on the first of those, and its owner guard on the last. Together the two replace the earlier single 47-file PR #2935, which can be closed.The design spec is committed at
docs/superpowers/specs/2026-07-27-clue-550-stage-3-sort-work-ui-design.mdand is the document to review alongside the code. Note that the spec records the design as it stood when written — several names changed during implementation (see "The axis-modeling checkpoint" below), anddocs/document-axes/is the current-state record. The roadmap atdocs/document-axes/README.mdflipskindto done and records the read side ofowner,container, andcurriculum.PR #2930 (CLUE-525) landed
canUserEditDocumenton master while this work was in flight, and the base branch has since merged it. This PR replaces its semantics, and the two disagree on a real case, so it is flagged here rather than left to be discovered in review.type === GroupDocumentconcurrentaxisgroupId === user.currentGroupIduser.userIdForGroupDocumentsThe group-membership row is the one that changes behavior. A group id is unique only within an offering, so
groupId === user.currentGroupIdmatches across offerings — group 3 of a past assignment is a different set of students, and its document still satisfies the test. That reaches real documents, because Sort Work's "All" filter lists everything a class has produced, including documents from offerings it has moved on from. Comparing owner uids is exact instead:group_<offeringId>_<groupId>carries the offering, so two same-numbered groups can never be confused.The merge resolution therefore removed CLUE-525's
describe("canUserEditDocument")block fromdocument-utils.test.ts: two of its cases assert the superseded contract, most directly "prefers the metadata groupId over the document's" → true, against this PR's "does not treat a document model's groupId as evidence of group ownership" → false. Every other case that block covered is covered here. The one case it uniquely had — a group document reached through thedocumentrather than its metadata — was ported to the new contract rather than dropped.The axis-modeling checkpoint, resolved
The project deferred "does scope need a helper, and if so what shape?" to its richest consumer. Working through Stage 3's five consumers produced two findings.
First, "scope" was three questions wearing one name. It has been split into
owner(who the document belongs to),container(where it is kept: class → classUnit → offering), andcurriculum(what it is about: nothing → unit → investigation → problem). Every consumer turned out to want exactly one of them, and picking the wrong one is a live defect rather than a style question:document.isGroupkind→ registry presentationtype === GroupDocumentconcurrent→ collaborative stylinguid === user.idconcurrent, then the document's owner and containerbyGroup/byNamesectioningtype === GroupDocumentThe listener is the sharpest illustration. It fetches documents about a whole unit, and it must not be shaped as a container question: an exemplar is kept in the classUnit container but is about a problem, so a container-shaped query would surface exemplars from every problem in a unit whenever a student was on any one of them.
Second, the answer is narrow named guards — no level enum and no unified struct.
src/models/document/document-axes.tsholdshasGroupOwner,hasClassOwner,isInClassUnitContainer, andgetCurriculumLabel, each reading only its own axis's stored fields — notype, nokind, no registry. That last point is load-bearing: Sort Work's "All" filter lists documents from other units, whose kinds were never registered this session, so a registry lookup would silently misfile them. The decision record, the field-by-shape table, and what is not covered yet are indocs/document-axes/reading-axes-in-code.md.On the creation side the kind registry declares
ownerType(user/group/class) andcontainerType(class/classUnit/offering). There is deliberately no group container level — a group document is kept in the offering alongside the problem documents its members write, and what makes it the group's is its owner.What's in the PR
Axes
document-axes.ts,document-kinds.ts), per the checkpoint above. On the creation side the kind registry declaresownerTypeandcontainerType; on the read side, narrow guards over stored fields.classUnitcontainer stampsinvestigation: null, problem: nullrather than omitting them, following the convention the rules already encode (hasPresentFieldtreats a null field as absent). Firestore cannot match a missing field, so this is what makes "about a unit but not a problem" queryable.uid, testing thegroup_prefix its minter used, symmetric withhasClassOwnerand itsclass_. That leaves theuidthe single authority on who a document belongs to; the storedgroupIdnow carries only Sort Work's group label and the two queries that retire with the canonical-pointer migration.Sort Work
unit in <variants> && investigation == null. It names notypeand nokind. Equality-only, so no composite index. It is a curriculum query, not a container one: an exemplar is kept in the classUnit container but is about a problem, so a container-shaped query would surface exemplars from every problem in a unit whenever a student was on any one of them.byGroupgets a "Whole Class" section ahead of the numbered groups;byNamefiles class-wide documents under "No Name". Ordering comes from structured sort keys instead ofparseInt-ing the display label — which mattered because the group term is translatable per unit and "Whole Class" contains no digits.byNameresolved a group document's members by bare group id. Group 3 of a past assignment is a different set of students, so its document was attributed to this offering's group 3 — or dropped from the sort when no group of that number existed here. Resolution now goes throughgetGroupByOwnerId, which builds the whole owner id and compares; a document from another offering gets its own section, labelled by where the work came from (Groups from sas-2.4).Workspace
Driving Question Board (sas-2.4)), and a unit-declared title is scoped to its own unit so two units declaring the same kind don't borrow each other's wording.canUserEditDocumentindocument-utils.ts, beside its siblingisDocumentAccessibleToUser, replacing two divergent inline gates (Sort Work's ownership-only check and the resources pane's tab check).Firestore rules
canonical/v1/classes/<classHash>/(offerings/<offeringId>|units/<unit>)/owners/<uid>/slots/<label>, built segment-by-segment from the document's own fields on both sides (getCanonicalPointerPathand the rules'canonicalPointerPath). Each container names only itself, so a new container level can be added later without moving pointers that already exist — they are immutable by rule. Taking the owner segment straight fromuidremoved the last reference togroupIdanywhere infirestore.rules.concurrentfield's write path (see below).Security: two defects found and fixed during review
Both were defects in the plan, caught before merge, and both have regression tests.
The edit predicate initially granted Edit on the user's own published documents. A publication's
uidis the publisher, so the ownership arm matched in the Class Work tab where the old tab-based gate had withheld it. Not a write hole (published documents are forced read-only downstream) but a dead button, and it contradicted a live Cypress assertion innav_panel_test_spec.js. Now excluded viaisPublishedType, with researchers denied explicitly.The history-write rule was forgeable. Gating on
concurrentwas unsound becauseconcurrentwas not a read-only field: any class member couldupdatea classmate's document with{concurrent: true}and thereby grant themselves — and the whole class — read and append on that document's private history. Fixed at the write path rather than the read path:concurrentChangeOk()permitsconcurrentto change only ontype == "group"documents, andtypeis itself read-only so it cannot be flipped first.That write allowance is transitional — it exists only because two backfill paths merge-update
concurrentonto existing group documents. Once the migration is complete it should be removed soconcurrentbecomes settable only at creation, and the create path should be constrained too (isValidDocumentCreateRequestcurrently checks neitherconcurrentnoruid). Breadcrumbs to that effect are infirestore.rules, the backfill script, and both design specs.Separately,
getDocumentOwnernow throws for an unregistered kind rather than defaulting to the creating user. Defaulting would hand a group's or a class's document to whoever created it and — because a canonical slot is addressed by its owner — file it in that user's slot instead of the shared one, both silently. Two existing tests were relying on the default; they now register the kind, as production does.Migration
scripts/backfill-group-concurrent.ts→scripts/backfill-group-document-axes.ts, now normalizing the stored axes of everytype: "group"document in two passes selected so they cover disjoint sets:groupId, missingconcurrent{ concurrent: true, kind: "group" }groupId, missing curriculum fields{ investigation: null, problem: null }Selecting on
groupIdrather than on the value being written matters: a class-wide document stampedkind: "group"would break both its title resolution and its canonical-pointer slot, since the slot label is the kind.Firestore rules deployment sequencing
The rules can be deployed before the code, in the usual order. Two parts of this change look at first like they might not allow that. Neither does, and the reasons are worth recording because both are non-obvious from the diff.
1. The history rule is purely additive.
The new arm is
isConcurrentClassDocument(), gated onconcurrent(not oncanonical). Pre-existing group documents don't carryconcurrentuntil a backfill runs, and the backfill only ships with this code — which reads like a catch-22 against a rules-first deploy: the rules would depend on a field nothing in production writes yet.It isn't one. Under the rules on production today, history create and read are gated by
isAuthed() && userOwnsDocument(), anduserOwnsDocument()compares the document'suidto the caller'splatform_user_id. Every group document'suidis the syntheticgroup_<offeringId>_<groupId>, which never equals a real user id — so no user can create or read group-document history in the authed domain today. The new arm can only widen that, and widens it to nothing whileconcurrentis absent. Deploying it early takes nothing away and enables nothing that is not already inert.2. The canonical-pointer path move denies a path production code still uses — and that is fine.
This move landed in Stage 2 rather than here, but none of Stages 1–3 is on production yet, so whichever deploy carries this branch carries it too.
Production code reads and writes pointers at
classes/<classHash>/offerings/<offeringId>/groups/<groupId>/canonical/<label>. These rules delete that match block and grant only thecanonical/v1/…/owners/<uid>/slots/<label>layout above; anything unmatched falls through to the top-level default-deny. Confirmed against the emulator — under these rules a class member's read and create at the legacy path are both denied. So between the rules deploy and the code release, a client on current code would take apermission-deniedon the first step ofgetOrCreateCanonicalDocument, thepointerRef.get(). Nothing catches it, sogetOrCreateGroupDocument()would reject.That costs nothing, because group documents are not released in the authed domains. They are opt-in per unit via
groupDocumentsEnabled, which defaults off; the only unit enabling it isdemo/units/qa, and thedemopartition has its own recursive-wildcardallow read, write: if isAuthed(), so it is unaffected by the authed pointer paths in either deploy order — as aredev,qa, andtest. The authed-domain group-document path is already non-functional, per the history rule above. Denying the legacy pointer path breaks it further, not something that works.So no dual-path rules release is needed. The rules do not have to carry both the legacy and the
canonical/v1pointer blocks through a transition window: deploy the rules, release the code after.Existing legacy-path pointers need no migration either. The new code never consults them — with no
v1pointer present,getOrCreateCanonicalDocumentfalls through tofindLegacyGroupDocument, which queries thedocumentscollection bycontext_id/offeringId/groupId, finds the same group document, and claims av1pointer for it. The document's existingcanonicallabel is rewritten to the same value, whichcanonicalFieldOkpermits as a no-op change. The old pointers are simply abandoned.Testing
Full suite green: 3591 Jest tests plus the emulator rules suite.
check:typesandlint:buildclean.New coverage includes the axis guards across every stored document shape, sectioning and sort-key ordering, the cross-offering group cases (attribution, the missing-group-number case, and section ordering), the whole-unit listener (registration per filter, dedupe, disposal, clearing on filter change), the edit predicate across all arms including negative cases, and rules tests for the owner segment of a canonical slot plus synthetic-owner history writes and the escalation that a class member cannot flip a classmate's document to concurrent.
Three verification notes, since none is visible in the diff:
--runInBand. The eight suites share one emulator and reset state per test, so running them in parallel produces ~51 spurious failures.firebase-test/package.json'stestscript does not pass the flag — pre-existing, and worth its own fix.canonicalPointerPathfails all three claim tests, and the guards' behavior tests fail when a guard is stubbed out.git diffbetween them empty), so nothing changed behaviorally in the reorganization. They have since diverged, as this branch has mergedmasterseveral times.Not done — for the reviewer
demo/units/qadeclares a slot today, so no production unit pays it. The fix is specified in the spec (defer the open, not the get-or-create) and wants its own ticket.groupIdis deliberately deferred. Nothing asks it the owner question anymore, butbyGroupstill uses it for a section label, and it does so in a way that intentionally ignores the offering: a group-3 document from a past assignment shares "Group 3" with this one's, which matches students carrying groups forward between offerings. Removing the field forces a behavior decision there, which is the real gate — not any remaining code cleanup.🤖 Generated with Claude Code