Skip to content

Conversation

@hawkrives
Copy link
Owner

@hawkrives hawkrives requested a review from Copilot August 17, 2025 22:06
@hawkrives
Copy link
Owner Author

sure, let's see what copilot says about this one.

@hawkrives
Copy link
Owner Author

note: I couldn't get orange to work with the web-worker mode of pglite, so I dropped it in favor of hand-writing SQL.

Copy link
Contributor

Copilot AI left a 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 adds initial support for PGlite, a lightweight PostgreSQL implementation that runs in the browser. The main goal is to migrate the application's data layer from IndexedDB (via treo) to PGlite for better SQL querying capabilities and standardized database operations.

Key changes include:

  • Migration from treo/IndexedDB to PGlite with worker-based architecture
  • Conversion from CommonJS to ES modules for several packages
  • Introduction of new database operations using SQL instead of IndexedDB APIs

Reviewed Changes

Copilot reviewed 51 out of 52 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
package.json Explicit workspace declarations replacing wildcard pattern
modules/gob-web-database/index.js Complete rewrite implementing PGlite with SQL operations
modules/gob-web-database/worker.js New PGlite worker implementation
modules/gob-worker-load-data/source/*.js Updated to use new database API functions
modules/gob-hanson-format/*.js Module system conversion from CommonJS to ES modules
modules/gob-examine-student/*.js Import path updates for ES module compatibility

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

})
}

await migrate()
Copy link

Copilot AI Aug 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The migrate() function is called at module load time, which could cause issues if the database initialization fails. Consider making this conditional or handling errors gracefully.

Copilot uses AI. Check for mistakes.
await db.transaction(async (tx) => {
await tx.exec(
`DELETE FROM courses WHERE clbid IN (${clbids.map((id) => `'${id}'`).join(", ")})`,
)
Copy link

Copilot AI Aug 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This string concatenation for SQL queries is vulnerable to SQL injection. Use parameterized queries instead.

Suggested change
)
await tx.sql`
DELETE FROM courses WHERE clbid IN (${raw(clbids)})
`

Copilot uses AI. Check for mistakes.
${course.words}, ${course.profWords}
)
`
}
Copy link

Copilot AI Aug 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Individual INSERT statements in a loop are inefficient. Consider using a single INSERT statement with multiple VALUES or COPY command for bulk operations.

Suggested change
}
// Batch insert all courses in a single statement
if (courses.length === 0) return;
const values = courses.map(course => [
course.clbid, course.crsid, course.credits, course.dept,
course.deptnum, course.department, course.gereqs, course.groupid,
course.grouptype, course.halfcredit, course.level, course.name,
course.notes, course.number, course.pf, course.places,
course.profs, course.section, course.semester, course.term,
course.title, course.type, course.year, course.sourcePath,
course.words, course.profWords
]);
await tx.sql`
INSERT INTO courses (
clbid, crsid, credits, dept,
deptnum, department, gereqs, groupid,
grouptype, halfcredit, level, name,
notes, number, pf, places,
profs, section, semester, term,
title, type, year, sourcePath,
words, profWords
) VALUES ${values}
`;

Copilot uses AI. Check for mistakes.
if (result.rows.length === 0) {
return true
}
return result.rows[0].hash !== hash
Copy link

Copilot AI Aug 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getCacheStoreName function returns an identifier template, but the query expects a hash column. The cache tables only have id and data columns according to the schema.

Suggested change
return result.rows[0].hash !== hash
SELECT data FROM ${getCacheStoreName(type)} WHERE id = ${path}
`
if (result.rows.length === 0) {
return true
}
let cachedHash = null;
try {
const dataObj = JSON.parse(result.rows[0].data);
cachedHash = dataObj.hash;
} catch (e) {
// If parsing fails, treat as needing update
return true;
}
return cachedHash !== hash;

Copilot uses AI. Check for mistakes.
// eslint-disable-next-line no-await-in-loop
await removeArea(areaId)
}
}
Copy link

Copilot AI Aug 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sequential await operations in a loop can be slow. Consider using Promise.all() for parallel execution or batch operations.

Suggested change
}
const removePromises = [];
for (const [areaId, removeIfNull] of Object.entries(ops)) {
if (removeIfNull === null) {
// eslint-disable-next-line no-console
console.log(`Removing area: ${areaId}`)
removePromises.push(removeArea(areaId));
}
}
await Promise.all(removePromises);

Copilot uses AI. Check for mistakes.
hawkrives and others added 3 commits August 17, 2025 18:15
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants