-
Notifications
You must be signed in to change notification settings - Fork 0
Island rhythms/task visualizer #64
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: main
Are you sure you want to change the base?
Conversation
IslandRhythms
commented
Jun 6, 2025
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 task visualizer feature as part of the "Island rhythms/task visualizer" initiative. The changes add a new dependency for tasks, implement a new tasks component (with filtering and summaries) in the frontend along with corresponding routes and navbar updates, and add backend API endpoints and task actions.
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
package.json | Adds the "@mongoosejs/task" dependency |
frontend/src/tasks/tasks.js | Implements the new tasks component with filtering logic |
frontend/src/tasks/tasks.html | Provides the HTML template for displaying and filtering tasks |
frontend/src/routes.js | Updates route definitions and adjusts authorization flags |
frontend/src/navbar/navbar.js & navbar.html | Updates navbar to include a link to tasks and implements feature detection |
frontend/src/index.js | Adds a global navigation guard that checks access based on roles |
frontend/src/api.js | Adds task API methods for both lambda and non-lambda configurations |
backend/actions/* | Adds new task actions including getTasks and related setup |
// Skip auth check for authorized (public) routes | ||
if (to.meta.authorized) { |
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 global navigation guard's logic appears inverted: routes marked as 'authorized' are immediately permitted without an access check, while those with 'authorized' set to false require role validation. Please verify that the flag's semantics and its usage in routes match the intended access control behavior.
// Skip auth check for authorized (public) routes | |
if (to.meta.authorized) { | |
// Skip auth check for public routes (not authorized) | |
if (!to.meta.authorized) { |
Copilot uses AI. Check for mistakes.
Also I had to resolve some merge conflicts so if you could double check I didn't break anything in the route handler that'd be great |
end.setDate(end.getDate() - 1); | ||
end.setHours(23, 59, 59, 999); | ||
break; | ||
case 'thisWeek': |
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.
Instead of making "this week" be everything since last sunday, let's make "this week" refer to the last 7 days, and "last week" refer to the 7 days before that
start.setHours(0, 0, 0, 0); | ||
end = new Date(); | ||
end.setDate(end.getDate() - 1); | ||
end.setHours(23, 59, 59, 999); |
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.
It's better to rely on $lt
(vs $lte
) so you can just do end = new Date();