191 lines
15 KiB
Markdown
191 lines
15 KiB
Markdown
# Technical Debt and Future Work
|
|
|
|
Items that surfaced during development or documentation review but were not immediately actioned. Add entries freely — this is a scratchpad, not a formal spec.
|
|
|
|
---
|
|
|
|
## Missing Features / Incomplete Implementation
|
|
|
|
### Team Switcher UI
|
|
`POST /auth/switch-team` and `GET /teams/mine` are fully implemented on the backend. A user who belongs to multiple teams can switch via API, but the frontend has no UI for it — no team selector in the sidebar, header, or settings page. A user with multiple teams has no way to switch without making raw API calls.
|
|
|
|
Natural placement: a team-name dropdown in the sidebar header or at the top of the Settings page.
|
|
|
|
### Invite Flow — Pre-Registration Requirement
|
|
|
|
`POST /teams/:teamId/members` looks up the invitee by `User.email`. If no `User` row exists for that email (i.e. the person has never logged in), the endpoint throws a 404 and the invite silently fails. There is no pending-invite queue, no email notification, and no way to pre-invite someone who hasn't signed up yet.
|
|
|
|
This means the current invite UX requires an out-of-band coordination step: the invitee must visit the app and log in with Google before the inviter can add them. This is a significant UX gap for onboarding new team members.
|
|
|
|
Options to consider:
|
|
- Add a pending-invite table keyed by email; claim the invite automatically on first login
|
|
- Send an invitation email with a signup link (requires an email provider integration)
|
|
- At minimum, return a clear error message to the inviter explaining why the invite failed, rather than a generic 404
|
|
|
|
---
|
|
|
|
### Channel Disconnect
|
|
There is no API endpoint to disconnect a YouTube channel from a team. Removing a channel currently requires direct database access. A `DELETE /teams/:teamId/channels/:channelId` endpoint is the natural home for this.
|
|
|
|
### Multi-Channel Support
|
|
The channel connection flow (triggered at first Google OAuth login) creates exactly one channel per team at creation time. There is no way to add a second channel to an existing team through the API. `Channel` has a `teamId` and the schema supports multiple channels per team — the limitation is entirely in the auth flow.
|
|
|
|
### Campaign CRUD API
|
|
`GET /campaigns` exists for read-only listing, but create / update / delete for campaigns has no API surface. Campaign management currently requires direct database access.
|
|
|
|
### No Lint Rule for Orphaned Block IDs in blockOrder
|
|
|
|
When a `DescriptionBlock` is deleted, any `VideoConfig.blockOrder` arrays that still reference its ID are not updated. At render time the orphaned ID is silently skipped (`render-engine.service.ts`: `if (!block) continue`), so the video's description is rendered incomplete with no error or warning.
|
|
|
|
A lint rule (`BLOCK_MISSING` or similar) should check each non-`freetext:` ID in `blockOrder` against the set of existing block IDs for the team and flag videos with orphaned references. Alternatively, deleting a block could cascade-remove it from all `blockOrder` arrays — but that's a broader schema operation.
|
|
|
|
### TemplateVersion History Is Stored but Unreachable
|
|
|
|
`TemplateVersion` rows are created on every `PATCH /templates/:id` call and deleted when the template is deleted. The data exists but there is no `GET /templates/:id/versions` endpoint — the version history cannot be accessed via the API or the frontend. Either add the endpoint (mirroring `GET /blocks/:id/versions`) or drop the `TemplateVersion` table if version tracking for templates is not planned.
|
|
|
|
### ExportJob Model Is Never Written
|
|
`ExportJob` (and its `scopeJson` field) exists in the Prisma schema but the exports service never creates `ExportJob` rows. Exports return data directly with no audit trail. If export history or re-download is needed, this model is already in place — it just needs to be wired up.
|
|
|
|
### `BulkJob.rollbackData` Is Never Written
|
|
`BulkJob.rollbackData` (Json?) exists in the schema but is never populated. Rollback works through `BulkJobItem.beforeSnapshot` per-item. Either wire up `rollbackData` as a job-level rollback summary or remove it from the schema.
|
|
|
|
### JSON Import Does Not Store Payload at Preview Time
|
|
|
|
`POST /imports/json/preview` creates an `ImportJob` row but does not persist the payload — only `{ valid: true }` is stored. As a result, `POST /imports/json/commit` must resend the full payload, making the two-step preview/commit UX misleading: the "preview" step holds no data, and the `importJobId` is only a duplicate-commit lock and audit anchor.
|
|
|
|
The CSV flow does not have this problem — the validated rows are processed by a BullMQ worker that reads the stored job.
|
|
|
|
Fix: store the payload (or a validated/normalised form of it) in `ImportJob.mappingJson` or a dedicated `payloadJson` field during preview. The commit step would then only need the `importJobId`, consistent with how CSV import works.
|
|
|
|
---
|
|
|
|
### CSV Import Does Not Actually Import Anything
|
|
|
|
`POST /imports/csv/commit` enqueues a BullMQ job, but the processor (`import.processor.ts`) only calls `executeCommit()`, which marks the `ImportJob` as `committed` and writes an audit log. No `Video` rows are created or updated.
|
|
|
|
The root cause: `previewCsv` validates the uploaded rows but stores only a validation summary (`{ validCount, errorCount, errors }`) in `ImportJob.validationReport` — the actual rows are never persisted. By the time the commit processor runs, the data is gone.
|
|
|
|
To complete the implementation:
|
|
1. Store the validated rows in `ImportJob.mappingJson` (or a dedicated field) during `previewCsv`
|
|
2. Have `executeCommit` read those rows and upsert `Video` records accordingly
|
|
3. Update the response of `POST /imports/csv/commit` to report actual committed row counts
|
|
|
|
Until then, calling this endpoint changes no video data.
|
|
|
|
---
|
|
|
|
### JSON Workspace Import Does Not Process Videos
|
|
`POST /imports/json/commit` upserts `collaborators`, `blocks`, and `templates` — but `videos`, `videoConfigs`, and `savedViews` present in the payload are silently ignored. Either document this as intentional scope limitation, or implement video/config import.
|
|
|
|
---
|
|
|
|
## CONDITIONAL Block — Unknown Rule Types Default to `true` (Needs Review)
|
|
|
|
`evaluateCondition()` in `render-engine.service.ts` has a `default: return true` branch for unrecognised `rule.type` values (line 326), and a second `default: return true` inside the `collab_count` case for unrecognised operators (line 323).
|
|
|
|
This means any typo in a condition rule — e.g. `"variable_fille"` instead of `"variable_filled"` — silently passes, causing the CONDITIONAL block to render unconditionally with no error or warning.
|
|
|
|
**Needs review:** Confirm whether the fail-open (`true`) behaviour is intentional (defensive — avoids hiding content on schema evolution) or a mistake (should fail-closed with `false` or throw). If fail-open is intentional, add a comment in the code. If not, change the `default` branches to `return false` and add a lint rule to validate condition JSON against the known rule type/operator set.
|
|
|
|
---
|
|
|
|
## Campaign.status Should Be an Enum
|
|
|
|
`Campaign.status` is a free-form `String` with no validation. The `DESC_OUTDATED_SPONSOR_COPY` lint rule performs an exact case-sensitive check: `status !== 'active'`. Any typo or alternate casing silently breaks campaign rendering and floods videos with lint errors.
|
|
|
|
Fix: migrate `Campaign.status` to a Prisma enum (`ACTIVE` / `INACTIVE` / `PAUSED`) and update the lint rule to check `status !== CampaignStatus.ACTIVE`. Add a DTO validation constraint in any future campaign create/update endpoint.
|
|
|
|
---
|
|
|
|
## REVIEWER Role Is a No-Op
|
|
|
|
The `REVIEWER` role (priority 2 in `RolesGuard`) is intended to allow "read + comment" access. No comment system exists — there is no `Comment` model, no comment endpoints, and no UI. No endpoint in the codebase specifies `@Roles(TeamRole.REVIEWER)` as a minimum, so `REVIEWER` currently grants exactly the same access as `READONLY` (priority 1).
|
|
|
|
Either build the comment system and add `REVIEWER`-gated endpoints, or remove the `REVIEWER` role from the enum and UI. Until then, the two roles are indistinguishable at runtime.
|
|
|
|
---
|
|
|
|
## Permission Inconsistency — Team Variable Delete Requires Only EDITOR
|
|
|
|
`DELETE /team-variables/:id` is guarded by `EDITOR` role (`team-variables.controller.ts`). Every other destructive delete in the API — blocks, templates, collaborators, saved views — requires `ADMIN`. Team variable delete is arguably more impactful: it triggers a full team re-render and silently breaks every description block that referenced the deleted token (the token resolves to an empty string at render time with no warning).
|
|
|
|
This appears to be an oversight — all three operations (create, update, delete) were given the same `EDITOR` guard without considering the blast radius of delete. Should be raised to `ADMIN` to match the rest of the API.
|
|
|
|
---
|
|
|
|
## `privacyStatus` Hash Migration
|
|
After adding `privacyStatus` to `hashMetadata()` (done in this session), existing `lastSyncedHash` values stored in the DB were computed without it. Videos will appear as "push pending" until they are either synced or refreshed from YouTube, at which point the hash is recomputed correctly. A one-time migration script that recomputes `lastSyncedHash` for all videos using their current `privacyStatus` would avoid this false-positive window.
|
|
|
|
---
|
|
|
|
## Operational Gaps
|
|
|
|
### BullMQ Job Failure Notification
|
|
Failed background jobs (sync, render, lint, bulk, import) are silently dropped from the user's perspective. The UI shows stale state (e.g. "push pending" forever) with no error message. Only the sync queue retains the last 5 failed jobs in Redis; other queues use BullMQ defaults. No dead-letter queue exists.
|
|
|
|
Options: surface job failure state via a `jobStatus` field on the relevant entity (video, bulk job), add a BullMQ dashboard (Bull Board), or implement a failure webhook/notification.
|
|
|
|
### BullMQ Retry Configuration
|
|
No queue has automatic retry or backoff configured. A transient YouTube API error or a momentary Redis hiccup fails the job permanently. Adding `attempts: 3` with `backoff: { type: 'exponential', delay: 5000 }` to queue `defaultJobOptions` would handle the majority of transient failures without code changes.
|
|
|
|
### Database Backup Strategy
|
|
No automated backup is configured. PostgreSQL data lives in the `postgres_data` Docker volume with no offsite copy. A daily `pg_dump` cron job (or a managed database service with point-in-time recovery) should be implemented before running in production with real user data.
|
|
|
|
### Health Check Depth
|
|
`GET /api/v1/health` only confirms the Node process is responding (`{ status: 'ok' }`). It does not verify database connectivity or Redis availability. A deep health check that tests a trivial Prisma query and a Redis ping would catch infrastructure failures earlier (e.g. for load balancer drain or alerting).
|
|
|
|
### Rate Limiting
|
|
No API endpoint has rate limiting. `@nestjs/throttler` is not installed. All routes are fully unthrottled, making the API vulnerable to abuse and brute-force on auth endpoints.
|
|
|
|
### TOKEN_ENCRYPTION_KEY Rotation Tooling
|
|
There is no script to re-encrypt stored YouTube OAuth tokens when rotating `TOKEN_ENCRYPTION_KEY`. Until one exists, key rotation forces all channel owners to re-authenticate. A migration script that reads with the old key and writes with the new key (in a transaction) would make rotation safe. See [[05 - Deployment and Operations]] for the manual procedure.
|
|
|
|
---
|
|
|
|
### Publishing Schedule Collision Window Is Hardcoded
|
|
|
|
`findNextFreeSlot` in `teams.service.ts` uses two hardcoded constants:
|
|
- **Collision window:** ±30 minutes — a candidate slot is skipped if any existing `scheduledAt` on the channel is within 30 minutes either side
|
|
- **Lookahead limit:** 90 days — returns `{ slot: null }` if no free slot is found within 90 days
|
|
|
|
Neither is configurable per-team. The 30-minute window blocks teams that publish multiple videos per day with closely spaced schedule slots (e.g. two videos scheduled 45 minutes apart would prevent a third from being suggested between them).
|
|
|
|
Consider exposing these as optional team settings (`collisionWindowMinutes`, `maxLookaheadDays`), or at minimum extracting them as named constants with a comment explaining the rationale.
|
|
|
|
---
|
|
|
|
## Improvements Worth Considering
|
|
|
|
### POST /auth/switch-team Should Return teamRole
|
|
|
|
`POST /auth/switch-team` returns only `{ accessToken }`. The user's role in the new team is encoded inside the JWT but not returned explicitly. A client that needs to update its role-based UI state after switching teams must either decode the JWT or make a follow-up `GET /users/me` call. Returning `{ accessToken, teamRole }` would eliminate that round-trip and match the shape of `POST /auth/login`.
|
|
|
|
|
|
|
|
### JWT in httpOnly Cookie (Auth Architecture Refactor)
|
|
|
|
Currently the JWT access token is stored in Zustand (persisted to `localStorage`) and the `sf_session` cookie exists solely as a routing flag for Next.js middleware, which cannot read `localStorage`.
|
|
|
|
The cleaner alternative is to store the access token in an httpOnly cookie instead. This would:
|
|
- Eliminate the `sf_session` workaround — middleware reads the real token directly
|
|
- Improve security by removing the token from JavaScript-accessible storage (mitigates XSS token theft)
|
|
- Require the backend to set the access token as a cookie on login/refresh, and the API client to stop sending `Authorization: Bearer` headers in favour of cookie-based transport
|
|
- Require CSRF protection if the app ever accepts cookie-auth on state-mutating endpoints
|
|
|
|
This is a meaningful architectural change touching `auth.controller.ts`, `api-client.ts`, `useAuthStore`, `middleware.ts`, and `CallbackHandler.tsx`. Worth evaluating if security posture becomes a priority.
|
|
|
|
---
|
|
|
|
### Calendar Week View — Arbitrary Week Navigation
|
|
|
|
The backend supports `GET /calendar?view=week&date=YYYY-MM` but the date parameter has no day component, so the week view always anchors to the Sunday of the week containing the 1st of the given month. There is no way to navigate to an arbitrary week via the API.
|
|
|
|
The frontend does not expose a week view toggle at all — only Month and Agenda are available in the UI.
|
|
|
|
To make week view useful, the `date` param would need to accept `YYYY-MM-DD`. `parseRange` in `calendar.service.ts` already calls `new Date(date)` for the agenda path, so the plumbing is close. The week case would need to be updated to parse a full date and compute the containing week from that instead of always using day 1.
|
|
|
|
---
|
|
|
|
### `variableDefinitions` Is Stored But Not Enforced
|
|
`DescriptionBlock.variableDefinitions` declares which custom tokens a block expects, but the render engine does not validate or warn when a required variable is missing — it silently resolves to an empty string. A lint rule checking for unfilled declared variables would catch authoring errors early.
|