Feature/lineage tracking #228
Conversation
…redos still work as expected
TrevorBurgoyne
added
refactor
joshua-dean
left a comment
joshua-dean
left a comment
There was a problem hiding this comment.
Good lift, just a couple of thoughts.
I wasn't incredibly thorough with every change happening in index.js, but if you'd like me to look at it (or anything in particular) with a fine-tooth comb, let me know and I'd be happy to break it down further.
| (cand.created_at === undefined) | ||
| ) { | ||
| cand.created_at = ULabel.get_time(); | ||
| } | ||
|
|
||
| // Add last edited by attribute if there is none | ||
| if ( | ||
| (cand.last_edited_by === undefined) | ||
| ) { | ||
| cand.last_edited_by = cand.created_by; | ||
| } | ||
|
|
||
| // Add last edited at attribute if there is none | ||
| if ( | ||
| (cand.last_edited_at === undefined) |
There was a problem hiding this comment.
I'm not entirely sure about this - the current behavior is that if annotations get loaded without this field, created_at gets the current time, and then last_edited_at copies that. created_by and last_edited_by occur in a similar manner.
Should these be set to null instead? last_edited_at and last_edited_by would get updated upon, but created_at and created_by would stay null forever, which is maybe not ideal but at least provides a method of identifying annotations created prior to this feature (or ones created externally that didn't populate this field).
I can see it either way, but in a more general sense I think if we are populating defaults, we should:
- Have a document somewhere that details what gets populated if not provided
- Consolidate the population of defaults to all be in
annotation.ts
Some of that implementation might be out of the scope of this feature, so I can open an issue to track that if necessary.
There was a problem hiding this comment.
Yes you are right, this is the resume from processing and so we shouldn't forcibly populate these fields.
And yeah, it would be good to open an issue for moving more of the annotation lifecycle into annotation.ts and actually using the ULabelAnnotation class more consistently in index.js.
| /*! For license information please see ulabel.js.LICENSE.txt */ | ||
|
|
There was a problem hiding this comment.
Is this referring to itself?
There was a problem hiding this comment.
yeah idk how this got added
There was a problem hiding this comment.
looks like its getting auto inserted by the build and I dont feel like tracking down why rn lol
src/actions.ts
Outdated
| on_start_annotation_spatial_modification( | ||
| ulabel, | ||
| action, | ||
| ); |
There was a problem hiding this comment.
It does feel slightly backwards to have the listeners/side effects always receive an action and then decide whether it matches - I think a lookup of side effects keyed by action types might make it easier to modify in the future. That would also provide a single spot to say "what happens when this action occurs (or is undone)", rather than "for what actions does this side effect occur".
Let me know what you think.
There was a problem hiding this comment.
Yeah originally I had multiple listeners triggering on certain actions, but I do prefer having each action map to specific listeners.
Here's the reworked version: ef87dd0
| undo() { | ||
| undo(this); | ||
| } | ||
|
|
||
| /** | ||
| * Redo the last undone action. | ||
| */ | ||
| redo() { | ||
| // Create constants for convenience | ||
| const current_subtask = this.get_current_subtask(); | ||
| const undone_stack = current_subtask["actions"]["undone_stack"]; | ||
|
|
||
| // If the action_steam is empty, then there are no actions to undo | ||
| if (undone_stack.length === 0) return; | ||
|
|
||
| this.redo_action(undone_stack.pop()); | ||
| redo(this); | ||
| } |
There was a problem hiding this comment.
We might want to name these in a less confusing manner.
There was a problem hiding this comment.
i really only kept ULabel.undo() and ULabel.redo() around on the off chance that people trigger them directly. They really shouldn't so maybe we just remove them altogether?
There was a problem hiding this comment.
Pull Request Overview
This PR implements lineage tracking for annotations and refactors action stream management. The main goal is to add minimal tracking of who last edited annotations and when, while also consolidating undo/redo functionality into a dedicated actions module.
- Added
last_edited_byandlast_edited_atfields to annotations for basic lineage tracking - Refactored action stream management into a centralized
actions.tsmodule with improved undo/redo flow - Enhanced error handling by replacing direct
alert,throw, andconsolecalls with standardizedlog_message()function
Reviewed Changes
Copilot reviewed 14 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/version.js | Version bump from 0.18.2 to 0.19.0 |
| src/toolbox_items/submit_buttons.ts | Added validation for annotation objects before processing |
| src/subtask.ts | Refactored type definitions and moved action types to external module |
| src/listeners.ts | Updated function call to use new standardized annotation creation method |
| src/error_logging.ts | Enhanced log_message function with optional alert suppression |
| src/annotation.ts | Added lineage tracking fields (last_edited_by, last_edited_at) |
| src/actions.ts | New comprehensive action management module with undo/redo functionality |
| package.json | Version bump to match src/version.js |
| demo/resume-from.html | Updated demo data format for consistency |
| demo/multi-class.html | Changed annotation scaling mode configuration |
| api_spec.md | Updated API documentation to reflect new annotation fields |
| CHANGELOG.md | Added release notes for version 0.19.0 |
| .github/tasks.md | New task tracking file for Copilot AI agent management |
| .github/copilot-instructions.md | New instruction file for Copilot AI agent configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Lineage Tracking + Actions Refactor
Description
copilot-instructionsand tested usingtasks.mdto manage a copilot AI agent to resolve an open github issue.actions.tsannotation_idout of undo/redo payloads and into the action itselflast_edited_byandlast_edited_atannotation fieldslast_edited_byor other minimal lineage tracking #218process_resume_fromnot actually populating missing annotation fieldsalert,throw, andconsolecalls withlog_message()suggest_edits()and other functions related to toggling dialogsPR Checklist
package.jsonhas been bumped since last releasepackage.jsonandsrc/version.jsnpm installandnpm run buildAFTER bumping the version numberapi_spec.md)changelog.mdBreaking API Changes
undo()andredo()can still be triggered from the ulabel instance. Other functions likerecord_actionhave moved intoactions.ts, but I don't think it's likely anyone was calling those directly.process_resume_frommean that all annotations loaded into ulabel could be modified, even if they are not edited by the user (e.g., to addlast_edited_byorcreated_byif not already present). imo this is how it was always intended to work, but if someone was depending on that broken behavior then be warned.