-
Notifications
You must be signed in to change notification settings - Fork 4
AI-generated content tile for exemplars #2626
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2626 +/- ##
==========================================
- Coverage 86.83% 86.73% -0.10%
==========================================
Files 783 789 +6
Lines 42342 42514 +172
Branches 10809 10836 +27
==========================================
+ Hits 36768 36876 +108
- Misses 5257 5320 +63
- Partials 317 318 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
collaborative-learning
|
Project |
collaborative-learning
|
Branch Review |
CLUE-240-ai-content
|
Run status |
|
Run duration | 19m 15s |
Commit |
|
Committer | Boris Goldowsky |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
0
|
|
2
|
|
4
|
|
0
|
|
149
|
View all changes introduced in this branch ↗︎ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new AI tile type for generating dynamic content in exemplars. The AI tile accepts a prompt and generates content that is displayed in the document, with responses cached in Firestore to improve performance.
- Adds AI tile registration and component infrastructure
- Implements Firebase functions for content generation and class data summarization
- Integrates OpenAI API for content generation with caching and locking mechanisms
Reviewed Changes
Copilot reviewed 33 out of 35 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
src/utilities/auth-utils.ts | Extracts production URL check into reusable function |
src/register-tile-types.ts | Registers new AI tile type for dynamic loading |
src/plugins/ai/*.ts | Core AI tile implementation with content model, component, and utilities |
src/components/document/summary-button.tsx | Adds UI component for viewing AI-generated class summaries |
shared/update-class-data-docs.ts | Refactors class data document processing for AI summarization |
functions-v2/src/*.ts | Implements Firebase functions for AI content generation and class data processing |
src/clue/app-config.json | Adds default AI system prompt configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
setLastUpdated(new Date(timestamp._seconds*1000)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The direct access to _seconds
suggests this is accessing a private Firebase Timestamp property. Consider using the public Timestamp API or converting the timestamp on the server side to avoid potential breaking changes if the internal structure changes.
setLastUpdated(new Date(timestamp._seconds*1000)); | |
} | |
// Use public API if available, fallback to _seconds if necessary | |
if (timestamp && typeof timestamp.toDate === "function") { | |
setLastUpdated(timestamp.toDate()); | |
} else if (timestamp && typeof timestamp._seconds === "number") { | |
setLastUpdated(new Date(timestamp._seconds * 1000)); | |
} |
Copilot uses AI. Check for mistakes.
const portals: string[] = ["learn.concord.org"]; // Consider all classes on production. | ||
const demos = ["AITEST"]; // Consider only classes in this demo area. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded portal and demo arrays limit flexibility. Consider making these configurable parameters or environment variables to support different deployment scenarios.
const portals: string[] = ["learn.concord.org"]; // Consider all classes on production. | |
const demos = ["AITEST"]; // Consider only classes in this demo area. | |
// Portals and demos can be configured via environment variables (comma-separated), or default to the current values. | |
const portals: string[] = process.env.PORTALS ? process.env.PORTALS.split(",").map(s => s.trim()) : ["learn.concord.org"]; // Consider all classes on production. | |
const demos: string[] = process.env.DEMOS ? process.env.DEMOS.split(",").map(s => s.trim()) : ["AITEST"]; // Consider only classes in this demo area. |
Copilot uses AI. Check for mistakes.
|
||
/** | ||
* Convert the AI tile content model to a Text tile content model. | ||
* The content of the Text tile will be the AI-generated content, converted to HTML for Slate. | ||
* @param content - The AI tile content model | ||
* @returns The Text tile content model | ||
*/ | ||
export function switchToTextContent(content: any) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter type any
reduces type safety. Consider defining a proper interface for the AI content or using the existing AIContentModelType.
/** | |
* Convert the AI tile content model to a Text tile content model. | |
* The content of the Text tile will be the AI-generated content, converted to HTML for Slate. | |
* @param content - The AI tile content model | |
* @returns The Text tile content model | |
*/ | |
export function switchToTextContent(content: any) { | |
import type { AIContentModelType } from "../../models/tiles/ai/ai-content"; | |
/** | |
* Convert the AI tile content model to a Text tile content model. | |
* The content of the Text tile will be the AI-generated content, converted to HTML for Slate. | |
* @param content - The AI tile content model | |
* @returns The Text tile content model | |
*/ | |
export function switchToTextContent(content: AIContentModelType) { |
Copilot uses AI. Check for mistakes.
@@ -173,7 +173,7 @@ There are a number of URL parameters that can aid in testing: | |||
|`studentDocumentHistoryId`|string |Open the history slider and move to the specified revision in the `studentDocument`.| | |||
|`portalDomain` |string |Used for dev/qa of standalone auth, this overrides which portal is used for auth| | |||
|`firebaseEnv` |`production`,`staging` |Target Firebase project for data and functions| | |||
|`showAiSummary` |`true` |When set to true the "ai summary" button in the document editor| | |||
|`showAiSummary` |`true` |When set to true the "ai summary" button in the document editor and Student Work view| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|`showAiSummary` |`true` |When set to true the "ai summary" button in the document editor and Student Work view| | |
|`showAiSummary` |`true` |When set to true show the "ai summary" button in the document editor and Student Work view| |
let errorMessage = ""; | ||
const lastUpdated = Timestamp.now(); | ||
try { | ||
const response = await openai.invoke(messages); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a limit to how long a firebase function can run? Will this return before that time runs out?
If this is an http request having it wait for multiple seconds while waiting for open ai will likely cause problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not noticed problems but I don't know what the limits are.
Component: AIComponent, | ||
tileEltClass: "ai-tool-tile", | ||
Icon, | ||
HeaderIcon: Icon // TODO: if this ever becomes a "real" tile (used in student/teacher work) we'll need a HeaderIcon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this tile used by teachers to configure a prompt for their student summary? So it is kind of used in teacher "work" if I'm understanding this correctly. Could you update this comment to clarify? Or maybe better would be to add some doc to the plugins/ai
which describes how this tile is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The canonical use is that these tiles are authored in curriculum (specifically, in Exemplars) and not available to students or teachers. When copied into a student/teacher document, the AI tile is transformed into a Text tile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good. I just a left a few comments. I'd guess the timeout issue will be a problem sometime in the future, but I don't think there is time to address it now.
If you could add some doc describing the point of the new ai tile that would be the most useful.
CLUE-240
Defines a new "AI" tile type. This is authored with a prompt, and generates content that is displayed in the document. The content is cached in Firestore.
settings.ai.systemPrompt
).showAiSummary=true
command line arg).at-midnight
to run.authorTools
section in unit configuration is respected. This would be where the AI tile toolbar button would normally be included.This adds NPM dependency
markdown-to-jsx
, so run npm install.Note the Firebase functions in this PR have already been deployed to production for testing.
Additional tests, including Cypress tests for the new tile type, would be nice to add to this, but we ran out of time.