CLUE-570: teacher-created custom comment tags

[lbondaryk]

Summary

Lets teachers add custom comment tags for their class (scoped to class + unit) that sync in real time and are immediately usable for manual comments, AI evaluation, and Sort Work grouping. Gated by a new unit authoring switch so legacy units are unaffected.

Out of scope (future): editing/combining/deleting tags without orphaning docs; Class-Summary tag suggestions.

Changes

Authoring

• New unit config allowCustomCommentTags (default false), threaded end-to-end like defaultSharedDocuments.
• New authoring Comments page (comments-settings.tsx) with the showCommentTag toggle (previously had no UI) and the allowCustomCommentTags switch; the comment roles / rating / tag-list editor moved here out of AI Settings. Registered in left-nav + workspace routing. AI Settings still mirrors AI categories from the tag list. No unit-JSON migration — only the authoring surface moved.

Sync (class + unit)

• CommentTags store + DBCommentTagsListener: Firestore onSnapshot at commentTags/{classHash}/units/{unit}/tags, reactively (re)subscribed as class/unit load.
• New firestore.rules entry grants in-class students read (teachers/researchers already covered by the parent /authed/{portal} rule).
• Effective tag list = unit-config tags + synced custom tags, used by the comment picker/display and Sort Work.
• getTagsWithDocs hardened so any tag on a document forms a group even when its label isn't loaded — no document is orphaned from the tag sort at Unit/All scope.

Add UI (Sort Work)

• Teacher-only "+ Add Tag" control below the "Not Tagged" section in the Strategy (tag) sort. Adding immediately creates a new empty sort section. <datalist> typeahead over existing tags; Add is disabled for a duplicate name (case-insensitive).

AI

• Custom tag ids are merged into aiPrompt.categories at the client's analysis-metadata write, so custom-evaluation units categorize with them. No functions-v2 change.

Test plan

• Unit tests added/updated: allowCustomCommentTags config getter, CommentTags store (merge/replace/id/path), getTagsWithDocs orphan guard, SortWorkAddTag (add + teacher gating + duplicate disable). tsc, ESLint, and affected Jest suites are green.
• Manual: unit with showCommentTag + allowCustomCommentTags on → teacher adds a tag in Sort Work → new empty section appears, tag is selectable in comments; tag a doc → it lands in that group. Demo mode (shared root) demonstrates teacher→student propagation locally.

Deploy notes

• Requires deploying the new Firestore rule (npm run deploy:firestore:rules) before students can read custom tags in authed mode.
• Cypress coverage for the Sort Work add-tag flow is not yet written (unit tests cover it).

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Adds support for teacher-created custom comment tags (scoped to class + unit) that sync in real time and are usable for comment tagging, Sort Work grouping, and AI categorization, with authoring UI moved to a new Comments settings page.

Changes:

• Introduces allowCustomCommentTags unit config and a new authoring “Comments” page; AI Settings now mirrors categories from tags.
• Implements a CommentTags store + Firestore snapshot listener for per-class/unit custom tags, and updates tag consumers to use merged (config + custom) tags.
• Adds a teacher-only “+ Add Tag” control in Sort Work and hardens tag grouping so documents with unknown tag ids still form groups.

Reviewed changes

Copilot reviewed 28 out of 28 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/utilities/sort-document-utils.ts	Prevents “orphaned” tagged docs by creating groups for unknown strategy/tag ids.
src/utilities/sort-document-utils.test.ts	Adds unit tests for getTagsWithDocs grouping + orphan guard.
src/models/stores/unit-configuration.ts	Adds allowCustomCommentTags to unit configuration schema.
src/models/stores/stores.ts	Registers new commentTags store in root stores.
src/models/stores/sorted-documents.ts	Uses merged tag list (config + custom) for sorting/grouping.
src/models/stores/document-group.ts	Uses merged tag list for Strategy grouping.
src/models/stores/document-group.test.ts	Updates store stubs to include commentTags.
src/models/stores/configuration-manager.ts	Adds computed getter for allowCustomCommentTags gated by showCommentTag.
src/models/stores/configuration-manager.test.ts	Adds tests for new config getter behavior.
src/models/stores/comment-tags.ts	New MobX store for custom tags + helper functions + Firestore write method.
src/models/stores/comment-tags.test.ts	Unit tests for id/path helpers and merge/replace behavior.
src/models/stores/base-stores-types.ts	Extends base store types to include commentTags.
src/models/stores/app-config-model.ts	Exposes allowCustomCommentTags via app config model.
src/lib/firebase.ts	Includes custom tag ids in AI categories when writing “custom” evaluation prompts.
src/lib/db-listeners/index.ts	Starts/stops the new Firestore comment-tags listener.
src/lib/db-listeners/db-comment-tags-listener.ts	New Firestore onSnapshot listener that syncs tag docs into the store.
src/components/document/sorted-section.test.tsx	Passes commentTags store into sorted-section test setup.
src/components/document/sort-work-view.tsx	Renders the new “Add Tag” control when sorting by Strategy.
src/components/document/sort-work-add-tag.tsx	New UI to add tags (teacher-only), with datalist suggestions + duplicate blocking.
src/components/document/sort-work-add-tag.test.tsx	Tests for teacher gating, add flow, and duplicate disable.
src/components/document/sort-work-add-tag.scss	Styles for the new “Add Tag” control.
src/components/chat/comment-card.tsx	Makes CommentCard reactive to merged tags so the picker includes custom tags.
src/authoring/types.ts	Adds allowCustomCommentTags to authoring unit config typing.
src/authoring/components/workspace/comments-settings.tsx	New authoring page for comment/tag settings; mirrors tags into AI categories.
src/authoring/components/workspace/ai-settings.tsx	Removes tag/role/rating authoring UI; keeps AI fields and mirrors categories from tags.
src/authoring/components/workspace.tsx	Adds routing to the new Comments settings page.
src/authoring/components/left-nav.tsx	Adds “Comments” entry in authoring left nav.
firestore.rules	Adds student read access to per-class/unit custom comment tags in authed mode.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[lbondaryk]

Fixed: the submit handler now only persists allowCustomCommentTags when showCommentTag is true, and deletes it otherwise.

[lbondaryk]

Fixed: the Add button is now also disabled when commentTagId(entered) matches an existing tag key (id collision), not just a matching display name. Added a test for the config-key-collision case.

[lbondaryk]

Added aria-label="New tag name" to the input.

[codecov]

Codecov Report

❌ Patch coverage is 93.37748% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.98%. Comparing base (1b061f5) to head (457f670).

Files with missing lines	Patch %	Lines
src/models/stores/comment-tags.ts	87.50%	4 Missing ⚠️
src/components/document/sort-work-add-tag.tsx	94.33%	3 Missing ⚠️
src/lib/db-listeners/db-comment-tags-listener.ts	94.28%	2 Missing ⚠️
src/models/stores/sorted-documents.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #2908    +/-   ##
========================================
  Coverage   85.98%   85.98%            
========================================
  Files         918      921     +3     
  Lines       52713    52854   +141     
  Branches    13940    13978    +38     
========================================
+ Hits        45324    45447   +123     
- Misses       7373     7391    +18     
  Partials       16       16            

Flag	Coverage Δ
cypress	?
cypress-regression	73.11% <52.27%> (-0.07%)	⬇️
cypress-smoke	42.01% <40.90%> (-0.01%)	⬇️
jest	54.97% <80.13%> (+0.06%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cypress]

collaborative-learning    Run #19363

Run Properties:   Passed #19363  •  457f670346: CLUE-570: trim redundant/obvious comments

Project	collaborative-learning
Branch Review	CLUE-570-user-created-comment-tags
Run status	Passed #19363
Run duration	10m 34s
Commit	457f670346: CLUE-570: trim redundant/obvious comments
Committer	Leslie Bondaryk
View all properties for this run ↗︎

---

Test results
Failures	0
Flaky	1
Pending	5
Skipped	0
Passing	220
View all changes introduced in this branch ↗︎

[dougmartin]

Nice work — the architecture here is clean: the CommentTags store / listener / merge split is easy to follow, the tag-id collision guard and the getTagsWithDocs orphan fix are thoughtful, typecheck is clean, and all the affected Jest suites pass for me.

Requesting changes on one blocker, though: the feature doesn't work in authed mode. The new Firestore rule grants only read, and parent-document rules don't cascade into subcollections, so the teacher's tag write is denied. I confirmed this against the rules emulator rather than inferring it. It passes manual testing only because demo mode is covered by the permissive /demo/{demoName}/{restOfPath=**} rule — and the failure is invisible in the UI, since the write isn't awaited or caught. Both fixes are small.

One other note: you also need to fix a merge conflict, probably from another PR that landed while you were developing this one.

Details below.

Findings

• firestore.rules:482-487 — BLOCKER: no write rule, so addTag() fails with PERMISSION_DENIED in authed mode.
  The new path grants only allow read. Firestore rules do not cascade into subcollections without a recursive wildcard, so the allow read, write on the parent match /authed/{portal} (line 123) does not apply to /authed/{portal}/commentTags/{classHash}/units/{unit}/tags/{tagId}. Confirmed against the rules emulator: a teacher write to that path fails with false for 'create' @ L12 (the catch-all deny). Reads behave correctly. Add an explicit write grant scoped to the teacher's class/unit, and validate the payload the way the existing comment/support rules do (id == tagId, classHash == classHash, unit == unit, createdBy is the requesting user, label is a non-empty string) so an in-class teacher can't write arbitrary or mismatched fields:

  match /commentTags/{classHash}/units/{unit}/tags/{tagId} {
    allow read: if isAuthed() && request.auth.token.class_hash == classHash;
    allow write: if isAuthedTeacher() &&
      (request.auth.token.class_hash == classHash || teacherIsInClass(classHash)) &&
      isValidCommentTagRequest(classHash, unit, tagId);
  }
  

  Also fix the comment above the rule, which asserts the (incorrect) parent-rule inheritance. Consider whether teachers/researchers viewing a different class of theirs need read access too — the current condition is class_hash == classHash only, whereas the neighboring aicontent rule (lines 475-479) also allows teacherIsInClass(classId).

• src/components/document/sort-work-add-tag.tsx:29-35 and src/models/stores/comment-tags.ts:51-66 — failed tag writes fail silently. commit() calls the async addTag() without awaiting or catching it, and addTag() has no error handling, so a rejected Firestore write surfaces only as an unhandled promise rejection while the UI clears the input and closes the row: the teacher sees nothing happen and gets no error. Make commit async, await the write, keep the entered text on failure, and show an error state.

• firebase-test/src/ — no rules test for the new path. Every other subcollection under /authed/{portal} has one (documents-rules.test.ts, supports-rules.test.ts, …). A comment-tags-rules.test.ts covering teacher-write / in-class-student-read / in-class-student-write-deny / out-of-class-student-read-deny would have caught the missing write rule.

• src/lib/db-listeners/db-comment-tags-listener.ts:26-33 — the reaction cannot observe a unit change. const { user, unit } = this.db.stores; captures the UnitModel instance; Stores.setUnit() (src/models/stores/stores.ts:324-326) replaces that instance wholesale, so a reaction reading unit.code off the captured object never re-fires when the unit changes. Stores is makeAutoObservable, so read through the store instead — () => ({ classHash: this.db.stores.user.classHash, unit: this.db.stores.unit.code }) — which makes the reassignment observable. Latent today (listeners start after unitLoadedPromise, and switching units does a full page reload), but the code comment claims the reaction "handles the unit loading asynchronously after listeners start", which is not true as written; without the fix, a unit change in place would leave the store subscribed to the previous unit's tags.

• src/utilities/sort-document-utils.ts:94-105 — nit: the orphan guard uses the raw tag id as the section label (tagValue: strategy), so an unresolved tag renders as a slug (e.g. my-custom-tag) in the Sort Work heading. Fine as a fallback, but worth confirming with design that a slug heading is acceptable versus something like "Unknown tag".

[lbondaryk]

Thanks for the thorough review — all addressed in 3018b16 (plus a master merge to clear the conflict).

• Blocker — firestore.rules missing write. Added an explicit allow write on commentTags/{classHash}/units/{unit}/tags/{tagId} (confirmed the parent /authed/{portal} rule doesn't cascade). Scoped per your recommendation to match the neighboring aicontent rule: teachers of the class or their networked classes (class_hash == classHash || teacherIsInClass(classHash)) can write, gated by a new isValidCommentTagRequest(classHash, unit, tagId) that checks id/classHash/unit match the path, createdBy == platform_user_id, and label is a non-empty string. Read now also allows networked teachers in addition to in-class users. Fixed the misleading comment. (Write stays teacher-only; researchers aren't granted, matching aicontent — flag if you want researcher read.)
• Silent failure. commit() is now async and awaits addTag(); on rejection it keeps the entered text, holds the row open, and shows a "Couldn't save tag — try again" error instead of clearing.
• Missing rules test. Added firebase-test/src/comment-tags-rules.test.ts: teacher-write, payload-validation rejects (bad createdBy / empty label / path mismatch), out-of-class-teacher write-deny, in-class-student read, in-class-student write-deny, out-of-class-student read-deny, and unauth/generic denies. I couldn't run it locally (no emulator here) — I'll verify on staging as usual.
• Listener reactivity. The reaction now reads this.db.stores.user.classHash / this.db.stores.unit.code through the observable Stores (was capturing the UnitModel instance, which setUnit() replaces), and the comment is corrected.
• Nit (slug heading). Leaving the raw-id fallback as-is — confirmed acceptable on our side.

tsc + ESLint clean; comment-tags / document-group / sort-work Jest suites pass.

[lbondaryk]

Follow-up (0f62c89):

• Researcher read — correction to my note above. Researchers can already read custom tags: the read rule's first clause isAuthed() && request.auth.token.class_hash == classHash is role-agnostic, so any in-class member (student, teacher, or researcher, all of whom carry class_hash == classHash) satisfies it, and the commenting tag picker (comment-card.tsx) isn't role-gated. So researchers see and use the tags when commenting with no rule change needed — I only made the rule comment explicit and added tests (in-class researcher read allowed; researcher write denied). Write remains teacher-only.
• Log event added. A teacher successfully creating a tag now logs CREATE_CUSTOM_COMMENT_TAG with the tag name ({ tagName, tagId, unit, classHash }).

I can't run the rules suite locally (no emulator), so I'll verify the write rule on staging as usual.

[dougmartin]

Re-reviewed — approving. All four items from my last pass are genuinely fixed, and I verified the blocker against the emulator rather than taking the description's word for it.

Verified

• Firestore write rule. Ran the new firebase-test/src/comment-tags-rules.test.ts under the rules emulator: all 12 cases pass, including the teacher write that previously failed with PERMISSION_DENIED. I also ran the full rules suite to make sure the new isValidCommentTagRequest and the edited match block didn't regress a neighboring rule — clean. Worth noting for the record, since the tests alone don't prove it: the rule requires createdBy == platform_user_id, and the client writes createdBy: user.id, which traces back to uidAsString in src/lib/auth.ts:279 — the same identity the existing comment/support rules match on. So the real client write satisfies the rule, not just the test's hand-built payload.
• Researcher read. Your correction is right: the read rule's first clause is role-agnostic, so an in-class researcher already satisfies it, and there's now a test asserting it.
• Listener reactivity. Correct fix — Stores calls makeAutoObservable(this) (stores.ts:124) and setUnit reassigns this.unit (line 324), so reading through this.db.stores.unit.code does make the unit swap observable.
• Silent failure. commit() now awaits, keeps the entered text, and surfaces an error, with a test covering the rejected-write path.
• Merge conflict cleared; tsc and ESLint clean; the affected Jest suites pass (64 tests).

Non-blocking nits — fine to fold into a follow-up:

• src/components/document/sort-work-add-tag.tsx:95-102 — the "Tag already exists" / "Couldn't save tag" messages are plain <span>s, so a screen reader user gets no announcement when a save fails or a duplicate blocks Add; they just find the button silently disabled. Add role="alert" to both spans (the repo already does this in group-management-modal.tsx and picked-up-tile-ghost.tsx), and consider aria-invalid / aria-describedby on the input at lines 65-79.
• src/models/stores/comment-tags.ts:53-57 and sort-work-add-tag.tsx:31-45 — addTag() returns early successfully when classHash or unit is missing, so commit() clears the input and closes the row as though the write happened. Throw (or return a boolean the caller checks) so it hits the same error state as a rejected write.
• src/components/document/sort-work-add-tag.tsx:39-41 — the catch swallows the rejection, so a PERMISSION_DENIED in the field leaves nothing in the console to diagnose. Log the caught error alongside setting the error state.
• firebase-test/src/comment-tags-rules.test.ts:28-52 — the networked-teacher branch (teacherIsInClass in firestore.rules:502 and 504) is untested; the write cases cover only an in-class teacher and an out-of-class teacher with no network. Add a case with a teacher whose network includes the class, mirroring the aicontent rules tests.

Don't forget the npm run deploy:firestore:rules step in your deploy notes — students and teachers both need the new rule live.

[lbondaryk]

Addressed the latest nits in a462464:

• A11y announcements — the "Tag already exists" / "Couldn't save tag" spans now have role="alert", and the input carries aria-invalid + aria-describedby, so a screen-reader user hears why Add is blocked/failed instead of finding the button silently disabled.
• Non-silent addTag — it now throws when classHash/unit is missing instead of returning as though the write happened, so commit() lands in the same error state as a rejected write (input kept, error shown).
• Error logging — commit()'s catch now console.errors the caught rejection so a PERMISSION_DENIED is diagnosable.
• Networked-teacher rules test — added write + read cases for a teacher whose teacherIsInClass grants access (seeding the class-teachers doc), covering the teacherIsInClass branch in the write/read rules.

Added unit coverage too: role="alert" + aria-invalid on the error path, and addTag rejecting when class/unit is unavailable. tsc + ESLint clean; sort-work-add-tag + comment-tags suites green.