feat: dashboard frontend layout — router, sidebar, auth, placeholder views#19
Merged
Conversation
Create api_tokens table for client token authentication with bcrypt hash storage, prefix-based lookup index, and usage tracking columns.
GORM-based store with Create, List, FindByPrefix, Revoke, IncrementStats, IncrementErrorCount, GetByID, and BatchIncrementStats methods. APIToken model added to models.go.
Handlers for POST /api/auth/login (master token validation + HMAC cookie), POST /api/auth/logout (cookie clear), GET /api/auth/me (status check), GET/POST /api/auth/tokens (list/create), DELETE /api/auth/tokens/:id (revoke). All handlers include swaggo annotations.
TokenAuth now supports three auth methods: master token (admin), client API tokens (eng_* prefix with bcrypt verification and scope enforcement), and HMAC-SHA256 signed session cookies (dashboard). Read-only tokens are restricted to GET + whitelisted POST endpoints. HMAC cookie key derived deterministically from master token via SHA-256.
Add public routes (login, logout) and authenticated routes (me, tokens CRUD) to setupRoutes(). Create TokenStore in initializeAsync() and wire it into TokenAuth middleware after DB initialization.
Background goroutine reads token IDs from a buffered channel and flushes accumulated request counts to the database every 5 seconds, reducing per-request UPDATE overhead for client token authentication.
New handlers in handlers_vault.go:
- GET /api/vault/credentials — list credentials (no values)
- GET /api/vault/credentials/{name} — get with decrypted value
- POST /api/vault/credentials — store credential
- DELETE /api/vault/credentials/{name} — delete credential
- GET /api/vault/status — vault encryption status
All handlers delegate to existing crypto.Vault and ObservationStore.
Includes swaggo annotations for OpenAPI docs.
New handlers in handlers_tags.go:
- POST /api/observations/{id}/tags — add/remove/set tags
- GET /api/observations/by-tag/{tag} — list observations by tag
Logic mirrors MCP tag_observation and get_observations_by_tag handlers.
Includes swaggo annotations for OpenAPI docs.
New handlers in handlers_sessions_rest.go: - GET /api/sessions-index — list indexed sessions - GET /api/sessions-index/search — full-text search transcripts Uses /api/sessions-index path to avoid conflict with existing /api/sessions live session management routes. Includes swaggo annotations for OpenAPI docs.
New handlers in handlers_maintenance.go: - POST /api/maintenance/consolidation — trigger consolidation cycle - POST /api/maintenance/run — trigger full maintenance - GET /api/maintenance/stats — maintenance statistics Delegates to existing consolidationScheduler and maintenanceService. Includes swaggo annotations for OpenAPI docs.
New handler in handlers_analytics.go: - GET /api/analytics/trends — temporal trends (obs per period) Supports daily/weekly/hourly grouping with project filter. Logic mirrors MCP get_temporal_trends handler. Includes swaggo annotations for OpenAPI docs.
Wire all new REST handlers into the requireReady + auth route group:
- /api/vault/* (5 endpoints)
- /api/observations/{id}/tags, /api/observations/by-tag/{tag}
- /api/sessions-index, /api/sessions-index/search
- /api/maintenance/* (3 endpoints)
- /api/analytics/trends
ОбзорРеализована комплексная система аутентификации и управления API-токенами с поддержкой сессионных cookies, клиентских токенов на основе bcrypt, и отслеживанием статистики использования. Добавлены REST-обработчики для управления учетными данными, тегами, сессиями и аналитикой. Расширен фронтенд Vue с маршрутизацией, системой входа и несколькими представлениями компонентов. Изменения
Диаграммы последовательностиsequenceDiagram
participant Client
participant API as API Service
participant TokenAuth as TokenAuth Middleware
participant TokenStore as Token Store
participant DB as Database
Client->>API: POST /api/auth/login<br/>(token)
API->>TokenAuth: validateMasterToken()
alt Token Valid
TokenAuth-->>API: authenticated
API-->>Client: 200 + Session Cookie
else Token Invalid
TokenAuth-->>API: failed
API-->>Client: 401 Unauthorized
end
Client->>API: GET /api/auth/me<br/>(with Session Cookie)
API->>TokenAuth: Middleware.authenticateSessionCookie()
TokenAuth-->>API: role extracted from context
API-->>Client: 200 + Auth Status + Role
sequenceDiagram
participant Client
participant API as API Service
participant ClientTokenAuth as ClientToken Auth
participant TokenStore as Token Store
participant DB as Database
Client->>API: GET /api/observations<br/>(Authorization: Bearer eng_xxx)
API->>ClientTokenAuth: authenticateClientToken()
ClientTokenAuth->>TokenStore: FindByPrefix(prefix)
TokenStore->>DB: Query api_tokens WHERE revoked=false
DB-->>TokenStore: APIToken record
TokenStore->>ClientTokenAuth: token + metadata
ClientTokenAuth->>ClientTokenAuth: bcrypt.CompareHash()
alt Hash Match & Not Revoked
ClientTokenAuth-->>API: authenticated + scope
alt Scope Check (read-only vs endpoint)
API-->>Client: 200 + Data
else Read-Only Violation
API-->>Client: 403 Forbidden
end
else Invalid or Revoked
ClientTokenAuth-->>API: failed
API-->>Client: 401 Unauthorized
end
sequenceDiagram
participant Service as Service
participant StatsCh as Stats Channel
participant TokenStatsFlusher as Token Stats Flusher
participant TokenStore as Token Store
participant DB as Database
Service->>StatsCh: emit token_id (async)
loop Every Request
StatsCh->>TokenStatsFlusher: receive token IDs
TokenStatsFlusher->>TokenStatsFlusher: batch by token_id<br/>(accumulate counts)
end
loop Every 5 seconds
TokenStatsFlusher->>TokenStore: BatchIncrementStats(counts)
TokenStore->>DB: UPDATE api_tokens<br/>SET request_count=+N,<br/>last_used_at=NOW()
DB-->>TokenStore: success
TokenStatsFlusher->>TokenStatsFlusher: clear counts
end
Service->>TokenStatsFlusher: cancel context
TokenStatsFlusher->>TokenStore: final flush
TokenStore->>DB: final updates
Оценка усилий при проверке кода🎯 4 (Сложный) | ⏱️ ~45 минут Предлагаемые метки
Стихотворение
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a significant overhaul of the Engram Dashboard's frontend and backend infrastructure, laying the groundwork for future features. It introduces a modern, modular Vue.js application with robust routing and authentication, alongside comprehensive API extensions for managing various system aspects like analytics, security tokens, and data maintenance. The changes aim to provide a more interactive and secure user experience while preparing the platform for scalable growth. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant set of features for the Engram Dashboard frontend, including a new layout with routing, a sidebar, and authentication. It also adds corresponding backend support with new API endpoints for authentication, token management, analytics, and more. The changes are extensive and well-structured, covering both backend Go services and the Vue.js frontend.
My review focuses on a few areas for improvement in the new backend handlers, particularly around analytics correctness, code idiomaticity, and user experience in search results. The new authentication flow is robust and secure. The frontend architecture with Vue Router and composables is a solid foundation for future development.
| } | ||
|
|
||
| // Get observations for analysis (rough estimate of limit) | ||
| obs, err := s.observationStore.GetRecentObservations(r.Context(), project, days*50) |
There was a problem hiding this comment.
The current implementation for fetching observations for trend analysis uses a "rough estimate" limit of days * 50. This can lead to inaccurate analytics if the actual number of observations exceeds this estimate, as some data points for the requested period will be missed. To ensure correctness, you should fetch all observations within the specified date range (startTime to now). This might require adding a new method to s.observationStore that accepts a time range.
Comment on lines
+126
to
+132
| for i := 0; i < len(topConcepts) && i < 10; i++ { | ||
| for j := i + 1; j < len(topConcepts); j++ { | ||
| if topConcepts[j].count > topConcepts[i].count { | ||
| topConcepts[i], topConcepts[j] = topConcepts[j], topConcepts[i] | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation uses a selection sort to find the top 10 concepts. While this works, it's not very idiomatic or efficient for larger datasets. You can achieve the same result with more readable and standard Go code by using sort.Slice from the standard library. You will need to add import "sort" to the file.
sort.Slice(topConcepts, func(i, j int) bool {
return topConcepts[i].count > topConcepts[j].count
})
Comment on lines
+348
to
+370
| // isDuplicateKeyError checks if the error is a unique constraint violation. | ||
| func isDuplicateKeyError(err error) bool { | ||
| if err == nil { | ||
| return false | ||
| } | ||
| msg := err.Error() | ||
| // PostgreSQL unique violation error code 23505 | ||
| return containsDuplicateKey(msg) | ||
| } | ||
|
|
||
| // containsDuplicateKey checks error message for duplicate key indicators. | ||
| func containsDuplicateKey(msg string) bool { | ||
| for _, s := range []string{"duplicate key", "23505", "UNIQUE constraint"} { | ||
| if len(msg) >= len(s) { | ||
| for i := 0; i <= len(msg)-len(s); i++ { | ||
| if msg[i:i+len(s)] == s { | ||
| return true | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
The function containsDuplicateKey manually implements a substring search, and isDuplicateKeyError just calls it. This can be simplified by using strings.Contains from the standard library directly inside isDuplicateKeyError and removing containsDuplicateKey. This will make the code more idiomatic and concise. Make sure to add import "strings" to the file.
// isDuplicateKeyError checks if the error is a unique constraint violation.
func isDuplicateKeyError(err error) bool {
if err == nil {
return false
}
msg := err.Error()
// PostgreSQL unique violation error code 23505
for _, s := range []string{"duplicate key", "23505", "UNIQUE constraint"} {
if strings.Contains(msg, s) {
return true
}
}
return false
}| if res.Session.Content.Valid && len(res.Session.Content.String) > 0 { | ||
| snippet := res.Session.Content.String | ||
| if len(snippet) > 200 { | ||
| snippet = snippet[:200] |
There was a problem hiding this comment.
When truncating the search result snippet, it's good practice to add an ellipsis (...) to indicate that the text has been shortened. This provides a better user experience by making it clear that there is more content available.
Suggested change
| snippet = snippet[:200] | |
| snippet = snippet[:200] + "..." |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/worker/service.go (1)
724-762:⚠️ Potential issue | 🟠 MajorОбновите tokenStore и graphStore в reinitializeDatabase так же, как в async init.
В
reinitializeDatabaseпосле создания новогоstoreнеобходимо воссоздать и переинициализироватьtokenStore, пересвязатьs.tokenAuth.SetTokenStore(), пересоздатьgraphStoreиgraphWriter, и перезапуститьs.startTokenStatsFlusher(). Без этого после пересоздания БД token auth и графовое хранилище будут использовать закрытые соединения.Изменения для добавления
// После создания нового store и перед mutex swap в reinitializeDatabase: tokenStore := gorm.NewTokenStore(store) // Переинициализировать graph store (аналогично async init) var gs graphpkg.GraphStore = &graphpkg.NoopGraphStore{} var gw *graphpkg.AsyncGraphWriter if s.config.GraphProvider == "falkordb" && s.config.FalkorDBAddr != "" { fdb, err := falkordb.NewFalkorDBGraphStore(s.config) if err != nil { log.Warn().Err(err).Msg("FalkorDB connection failed after reinit, falling back to noop") } else { gs = fdb gw = graphpkg.NewAsyncGraphWriter(gs) relationStore.SetCallback(gw.Enqueue) } } // В mutex swap добавить: s.tokenStore = tokenStore s.graphStore = gs s.graphWriter = gw // После unlock вызвать: if s.tokenAuth != nil { s.tokenAuth.SetTokenStore(tokenStore) } s.startTokenStatsFlusher(s.ctx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/service.go` around lines 724 - 762, reinitializeDatabase currently recreates the DB `store` but doesn't reinitialize the dependent components (tokenStore, graphStore/graphWriter) so they keep using closed connections; after creating the new `store` in `reinitializeDatabase` create `tokenStore := gorm.NewTokenStore(store)`, reinitialize the graph backend exactly like the async init (build `gs` as a `graphpkg.GraphStore` fallback to `&graphpkg.NoopGraphStore{}`, try `falkordb.NewFalkorDBGraphStore(s.config)` and on success create `gw := graphpkg.NewAsyncGraphWriter(gs)` and `relationStore.SetCallback(gw.Enqueue)`), then inside the mutex swap assign `s.tokenStore = tokenStore`, `s.graphStore = gs`, and `s.graphWriter = gw`; after unlocking call `if s.tokenAuth != nil { s.tokenAuth.SetTokenStore(tokenStore) }` and `s.startTokenStatsFlusher(s.ctx)` so auth and the token flusher use the new connections.
🟡 Minor comments (4)
ui/src/views/VaultView.vue-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorСкройте декоративную иконку от скринридеров.
Сейчас иконка может быть озвучена как содержательный элемент, хотя она декоративная.💡 Предлагаемое исправление
- <i class="fas fa-vault text-4xl mb-4 block" /> + <i class="fas fa-vault text-4xl mb-4 block" aria-hidden="true" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/views/VaultView.vue` at line 3, The vault icon element (<i class="fas fa-vault text-4xl mb-4 block" />) is decorative and should be hidden from screen readers; update the element to include accessibility attributes (add aria-hidden="true" and focusable="false" if supported) so it is ignored by assistive technologies, ensuring any meaningful text remains available elsewhere in the component (e.g., keep visible heading or label elements like VaultView title).ui/src/views/LoginView.vue-62-76 (1)
62-76:⚠️ Potential issue | 🟡 MinorСвяжите сообщение об ошибке с полем токена.
Сейчас ошибка логина показывается только визуально: инпут не получает
aria-invalid/aria-errormessage, а сообщение не оформлено как live region. Из-за этого причина отказа после submit может не озвучиваться скринридером. (developer.mozilla.org)💡 Вариант правки
<input id="token-input" v-model="token" type="password" @@ autocomplete="current-password" class="w-full px-4 py-3 rounded-lg bg-slate-900/50 border border-slate-600/50 text-white placeholder-slate-500 focus:outline-none focus:ring-2 focus:ring-claude-500/50 focus:border-claude-500 transition-colors" :disabled="submitting" + :aria-invalid="error ? 'true' : 'false'" + :aria-errormessage="error ? 'token-input-error' : undefined" /> <!-- Error message --> - <p v-if="error" class="mt-3 text-sm text-red-400"> - <i class="fas fa-exclamation-circle mr-1" /> - {{ error }} - </p> + <p + id="token-input-error" + role="alert" + :class="error ? 'mt-3 text-sm text-red-400' : 'sr-only'" + > + <template v-if="error"> + <i class="fas fa-exclamation-circle mr-1" /> + {{ error }} + </template> + </p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/views/LoginView.vue` around lines 62 - 76, Add proper ARIA linking between the token input and the error message: on the input with id "token-input" and v-model "token" bind aria-invalid to the error presence (e.g., :aria-invalid="Boolean(error)") and set aria-errormessage to the error element id (e.g., :aria-errormessage="error ? 'token-error' : undefined"); change the error <p v-if="error"> to have a stable id "token-error" and include role="alert" and aria-live="assertive" so screen readers announce the message (ensure the id exists only when showing the error or adjust to v-show if you need the id present while hidden).internal/worker/handlers_tags.go-156-174 (1)
156-174:⚠️ Potential issue | 🟡 MinorПараметр
offsetзадокументирован, но фактически игнорируется.Сейчас endpoint всегда возвращает первую страницу. Это ломает ожидаемую пагинацию для UI.
💡 Предлагаемое исправление
limit := 50 + offset := 0 if val := r.URL.Query().Get("limit"); val != "" { if parsed, err := strconv.Atoi(val); err == nil && parsed > 0 && parsed <= 200 { limit = parsed } } + if val := r.URL.Query().Get("offset"); val != "" { + if parsed, err := strconv.Atoi(val); err == nil && parsed >= 0 { + offset = parsed + } + } @@ searchParams := search.SearchParams{ Query: tag, Type: "observations", Project: project, Limit: limit, + Offset: offset, Concepts: tag, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/handlers_tags.go` around lines 156 - 174, The handler currently ignores the documented "offset" query parameter causing pagination to always return the first page; parse r.URL.Query().Get("offset"), convert to an int (default 0), validate it is >= 0 (and apply any project-specific max if desired), and set that value into the search parameters before calling the search manager (assign to search.SearchParams.Offset). Update the parsing logic near the existing limit handling (where limit is set and searchParams is built) so the offset variable is used when constructing searchParams in handlers_tags.go.internal/worker/handlers_auth.go-318-320 (1)
318-320:⚠️ Potential issue | 🟡 MinorНе сводите любой
Revoke()error к404.Даже после фикса store этот код продолжит превращать реальные ошибки БД/сети в
not found. Клиент потеряет различие между отсутствующим токеном и внутренним сбоем.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/handlers_auth.go` around lines 318 - 320, The current handler turns every error from tokenStore.Revoke into a 404; change it to distinguish not-found from other failures by using the store's sentinel error (e.g. errors.Is(err, store.ErrNotFound) or the token store's specific NotFound error) after calling tokenStore.Revoke(r.Context(), id). If the error is the not-found sentinel, keep returning http.StatusNotFound; for any other error, log it (using log.Error().Err(err).Str("token_id", id).Msg(...)) and return http.StatusInternalServerError (500) with an appropriate message. Ensure you reference tokenStore.Revoke, the error variable from that call, and the http.Error(w, ...) response paths when making the change.
🧹 Nitpick comments (3)
ui/src/views/AnalyticsView.vue (1)
2-6: Вынесите повторяющийся placeholder-шаблон в общий компонент.
Сейчас одинаковая структура дублируется в нескольких view, что повышает стоимость правок.♻️ Вариант рефактора
+<script setup lang="ts"> +import ComingSoonView from '@/components/ComingSoonView.vue' +</script> + <template> - <div class="text-slate-400 text-center mt-20"> - <i class="fas fa-chart-line text-4xl mb-4 block" /> - <h2 class="text-xl font-medium text-slate-300">Analytics</h2> - <p class="mt-2 text-sm">Usage analytics and trends. Coming in Phase 11.</p> - </div> + <ComingSoonView + icon="fa-chart-line" + title="Analytics" + description="Usage analytics and trends. Coming in Phase 11." + /> </template>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/views/AnalyticsView.vue` around lines 2 - 6, Extract the repeated placeholder markup into a reusable Vue component (e.g., EmptyPlaceholder or PlaceholderView) that accepts props for icon (prop name: iconClass), title (prop name: title), and description (prop name: description), implement the template using the existing structure and classes, export it, and then replace the duplicated block in AnalyticsView.vue by importing and using <EmptyPlaceholder :iconClass="'fas fa-chart-line'" :title="'Analytics'" :description="'Usage analytics and trends. Coming in Phase 11.'"/>; update other views that use the same markup to consume this new component as well.internal/worker/middleware.go (1)
253-257: Session-cookie сейчас всегда превращается вadmin, независимо от содержимого.
authenticateSessionCookie()проверяет только подпись и срок жизни, аMiddleware()всегда кладёт в context строку"admin". Либо возвращайте роль из cookie, либо явно отклоняйте всё, кромеadmin, иначе полеRoleсоздаёт fail-open семантику.Also applies to: 326-359
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/worker/middleware.go` around lines 253 - 257, Middleware currently maps any valid session cookie to the "admin" role, causing a fail-open; update the logic in Middleware where it reads the cookie (see authenticateSessionCookie and authRoleKey usage) so that authenticateSessionCookie returns the authenticated role (or additional metadata) instead of a boolean, and then set ctx with that returned role only if it is explicitly allowed (e.g., equals "admin") — alternatively, if you keep authenticateSessionCookie as-is, fetch and validate the role stored inside the cookie payload after signature/expiry verification and reject cookies that do not assert "admin" rather than always writing "admin" into context; apply the same change to the other Middleware block referenced (lines ~326-359).internal/db/gorm/token_store.go (1)
49-60: Поиск по короткому префиксу токена рано или поздно даст коллизию.
FindByPrefix()выбирает первую активную запись по префиксу, а генератор вinternal/worker/handlers_auth.go, Lines 35-36 и 261-263, использует только 8 hex-символов. При коллизии валидный token будет сверяться не с тем bcrypt hash и начнёт отклоняться, поэтому для активных токенов нужен более длинный и уникально индексируемый ключ поиска.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/db/gorm/token_store.go` around lines 49 - 60, FindByPrefix in TokenStore returns the first active row for an 8-char prefix which will eventually collide with the short generator in internal/worker/handlers_auth.go; change the lookup to use a longer, uniquely indexed search key instead of the 8-char prefix: add/use a dedicated unique searchable column on APIToken (e.g., token_key or full_token_id) with a DB unique/index constraint and update TokenStore.FindByPrefix to query that field (or require a longer prefix length consistent with the generator), and ensure the token generator produces and stores that longer unique key so bcrypt comparisons always target the correct token record.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/db/gorm/migrations.go`:
- Around line 1217-1230: The api_tokens table allows duplicate active
token_prefix values because the current non-unique index idx_api_tokens_prefix
doesn't enforce uniqueness; change the migration to create a unique partial
index on token_prefix for non-revoked rows (e.g., replace `CREATE INDEX IF NOT
EXISTS idx_api_tokens_prefix ...` with `CREATE UNIQUE INDEX IF NOT EXISTS
idx_api_tokens_prefix_unique ON api_tokens (token_prefix) WHERE NOT revoked`) so
lookups by prefix are unambiguous; ensure you remove or replace the old
non-unique index entry in the migration statements (in the block that defines
the api_tokens table/indexes in migrations.go) to avoid conflicting indexes.
In `@internal/db/gorm/token_store.go`:
- Around line 64-76: The Revoke method currently checks result.RowsAffected
before inspecting the update error, which can mask DB errors as a not-found;
change TokenStore.Revoke (the call starting with
s.db.WithContext(ctx).Model(&APIToken{}).Where(...).Updates(...)) to first check
result.Error and return it if non-nil, and only after a nil error check
result.RowsAffected and return gorm.ErrRecordNotFound when RowsAffected == 0;
keep the "revoked" and "revoked_at" Updates logic unchanged.
In `@internal/worker/handlers_analytics.go`:
- Around line 60-62: The code is using a rough cap days*50 when calling
s.observationStore.GetRecentObservations which can truncate data and skew
analytics; change the call in the analytics handler to fetch all relevant
observations (remove the days*50 cap) or implement proper pagination/streaming:
update the usage of s.observationStore.GetRecentObservations (or replace with a
new method such as GetObservationsSince/GetAllRecentObservations) to accept no
artificial small limit or iterate pages until exhausted, ensuring the full set
of observations for the requested range is retrieved before computing trend
metrics.
In `@internal/worker/handlers_auth.go`:
- Around line 95-102: The session cookie is set without the Secure flag; update
the http.SetCookie call that creates the admin session (where sessionCookieName,
cookieValue, sessionMaxAge are used) to set Secure: true for production, and
ensure the cookie-clearing path uses the same Secure, Path, HttpOnly, SameSite
attributes; detect TLS/proxy by checking request.TLS != nil or X-Forwarded-Proto
== "https" and only omit Secure for local non-TLS development.
- Around line 334-338: getAuthRole currently defaults to "admin" when
authRoleKey is missing, silently elevating privileges; change it to return an
empty string instead so callers must handle missing role explicitly. Update the
getAuthRole(r *http.Request) function to detect absence of authRoleKey{} and
return "" (empty string) rather than "admin", and ensure any callers of
getAuthRole (e.g., auth-helper usage sites) handle the empty-role case as
needed.
In `@internal/worker/handlers_maintenance.go`:
- Around line 82-93: The handler handleRunMaintenance uses the incoming HTTP
request context which is cancelled when the handler returns, causing background
work to be aborted; change the call s.maintenanceService.RunNow(r.Context()) to
use a non-cancelable context (context.Background()) so the maintenance goroutine
can run to completion, and apply the same change for the analogous call in
internal/mcp/server.go (replace any request-bound context passed into RunNow
with context.Background()).
In `@internal/worker/handlers_sessions_rest.go`:
- Around line 32-43: Both handlers accept unbounded positive "limit" values
which can trigger expensive queries and huge JSON responses; introduce a
constant (e.g., MaxLimit = 100 or appropriate safe max) and clamp any parsed
limit to that maximum instead of allowing arbitrary values. Update the parsing
logic around the limit variable (the blocks that set limit in
internal/worker/handlers_sessions_rest.go and the similar block at the later
occurrence) to: parse as before, then if parsed > MaxLimit set limit = MaxLimit
(or return a 400 if you prefer strict rejection), and use that clamped value for
subsequent list/FTS calls; keep offset parsing unchanged. Ensure the new
MaxLimit constant is referenced by both handlers so behavior is predictable and
enforced consistently.
- Around line 98-100: Handler currently documents a query param "project" but
never reads or forwards it to the backend; update the request handling code to
read c.Query("project") (or the equivalent request-query accessor used in this
file), validate/normalize it, and pass it into SearchSessions (or into the
SearchSessions options/filters struct) so the backend filtering includes
project; adjust the SearchSessions call/signature or build a filter object
(e.g., SearchOptions/Filter) to include Project and ensure the search
implementation uses that field when querying sessions.
In `@internal/worker/handlers_tags.go`:
- Around line 189-196: The current filtering in handlers_tags.go only checks for
concepts as []any which misses []string or plain string values and causes silent
failures; replace the single-type assertion with a type switch on
res.Metadata["concepts"] (handle []any, []string, and string) and in each branch
iterate/compare against tag, appending res to filtered and breaking once matched
(mirror the robust pattern used in internal/mcp/tools_memory.go around the type
switch), and apply the same fix pattern to the analogous logic in
internal/mcp/server.go that handles concepts so all possible metadata types are
covered.
In `@internal/worker/handlers_vault.go`:
- Around line 181-184: When req.Scope == "global" you must not persist
req.Project into the storage: before calling StoreObservation() clear or omit
the project field so the stored record is truly global; update the handler code
paths that call StoreObservation() (references: req.Scope, req.Project,
StoreObservation) to pass an empty project or a separate payload without project
when scope == "global" (also apply the same change to the other call site around
lines 211-213) so list/get/delete semantics remain consistent.
- Around line 93-97: The credential name (req.Name / chi.URLParam(r, "name"))
must be restricted to a single safe URL segment; add validation in the
credential creation handler to ensure req.Name matches a tight allowed pattern
(e.g. /^[A-Za-z0-9._-]+$/ or similar) and reject requests with slashes, question
marks or other unsafe characters with http.StatusBadRequest, and add the same
validation in the handlers that read/delete by chi.URLParam(r,"name") (the
read/delete handlers referenced) so the name used in the URL is guaranteed safe;
alternatively, if you prefer lookup by ID, change the create path to return a
generated ID and update read/delete to use that ID instead.
In `@internal/worker/token_stats.go`:
- Line 70: The call to BatchIncrementStats uses context.Background() without a
deadline which can block shutdown; replace it by creating a cancellable context
with a reasonable timeout (e.g., context.WithTimeout(..., 5*time.Second)), defer
the cancel(), and pass that ctx into store.BatchIncrementStats so the DB write
will fail fast during shutdown or DB degradation; update the call site where
BatchIncrementStats(context.Background(), counts) appears to use the new ctx and
consider making the timeout configurable if needed.
- Around line 42-43: В паттерне чтения из канала ch в функции/блоке где
происходит case tokenID := <-ch нужно обрабатывать закрытие канала через form
tokenID, ok := <-ch и при ok==false выйти из цикла/горутины, чтобы не
инкрементировать пустые токены в pending (map pending[tokenID]++), и избегать
утечек; также в функции flushTokenStats замените прямой вызов
context.Background() на контекст с таймаутом (например ctx, cancel :=
context.WithTimeout(parentCtx, timeout); defer cancel()) и используйте ctx при
запросах в базу, чтобы обеспечить корректный graceful shutdown при зависаниях
БД.
In `@ui/src/components/layout/AppHeader.vue`:
- Around line 24-26: The current flow always calls globalThis.location.reload()
even if fetch('/api/update/restart') returns non-ok or waitForWorker() fails
silently; change the logic so you first inspect the Response from
fetch('/api/update/restart') (the request to '/api/update/restart') and bail
(log/show error) if !response.ok, then ensure waitForWorker() either throws on
timeout or returns a boolean and check its success value before calling
globalThis.location.reload(); update both occurrences using waitForWorker and
globalThis.location.reload (lines around the fetch + reload blocks) so reload
only happens when the restart response is ok and waitForWorker reports success.
In `@ui/src/components/layout/AppSidebar.vue`:
- Around line 76-90: When collapsed is true the label text is removed via the
span's v-if, harming keyboard and screen-reader access; update the router-link
rendering in AppSidebar.vue so each navigation item still exposes accessible
text by replacing the conditional removal with a visually-hidden fallback and/or
an aria-label: keep the <span> for item.label but render it with a "sr-only"
class (or similar) when collapsed, and also add aria-label="{{ item.label }}" on
the router-link (or ensure isActive(item) logic still applies), so navItems'
item.label remains available to assistive tech while icons remain visible.
In `@ui/src/components/layout/Pagination.vue`:
- Around line 69-75: Add proper button semantics to the Pagination.vue icon-only
controls: set type="button" on the Previous and Next <button> elements to
prevent accidental form submission, and provide accessible names (e.g.,
aria-label="Previous page" and aria-label="Next page" or include visually-hidden
text) so the icon-only buttons are announced by screen readers; update the
buttons that call goToPage(currentPage - 1) and goToPage(currentPage + 1) (and
any similar page buttons between lines 80-101) accordingly.
- Around line 14-18: Normalize and clamp props.limit and props.offset before
deriving pagination values: create normalizedLimit = Math.max(1,
Math.floor(props.limit || 1)) and normalizedOffset = Math.max(0,
Math.min(Math.floor(props.offset || 0), Math.max(0, props.total -
normalizedLimit))); then use normalizedLimit/normalizedOffset in the
computations for totalPages, currentPage, showingFrom, showingTo and any
visiblePages logic so you never get Infinity or out-of-range pages; update
references to use those normalized identifiers (totalPages, currentPage,
showingFrom, showingTo, visiblePages) throughout the component.
In `@ui/src/composables/useAuth.ts`:
- Around line 20-27: The login() function can throw on network errors because
fetch is unhandled; wrap the fetch call in a try/catch (similar to checkAuth())
so any exceptions set authenticated.value = false and the function returns
false; on error optionally log the error for debugging and only set
authenticated.value = true when res.ok is true.
In `@ui/src/router/index.ts`:
- Around line 79-87: The route guard in router.beforeEach uses useAuth() but
only calls checkAuth() when loading.value is true, so subsequent navigations to
protected routes can skip re-checking and cause false positives; change the
logic so that for non-public routes (to.meta.public falsy) you always await
checkAuth() when either loading.value is true OR authenticated.value is false,
then re-evaluate authenticated.value and return { name: 'login' } only if it
remains false; update the guard around router.beforeEach, useAuth,
loading.value, checkAuth and authenticated.value accordingly so the auth state
is refreshed before blocking navigation.
---
Outside diff comments:
In `@internal/worker/service.go`:
- Around line 724-762: reinitializeDatabase currently recreates the DB `store`
but doesn't reinitialize the dependent components (tokenStore,
graphStore/graphWriter) so they keep using closed connections; after creating
the new `store` in `reinitializeDatabase` create `tokenStore :=
gorm.NewTokenStore(store)`, reinitialize the graph backend exactly like the
async init (build `gs` as a `graphpkg.GraphStore` fallback to
`&graphpkg.NoopGraphStore{}`, try `falkordb.NewFalkorDBGraphStore(s.config)` and
on success create `gw := graphpkg.NewAsyncGraphWriter(gs)` and
`relationStore.SetCallback(gw.Enqueue)`), then inside the mutex swap assign
`s.tokenStore = tokenStore`, `s.graphStore = gs`, and `s.graphWriter = gw`;
after unlocking call `if s.tokenAuth != nil {
s.tokenAuth.SetTokenStore(tokenStore) }` and `s.startTokenStatsFlusher(s.ctx)`
so auth and the token flusher use the new connections.
---
Minor comments:
In `@internal/worker/handlers_auth.go`:
- Around line 318-320: The current handler turns every error from
tokenStore.Revoke into a 404; change it to distinguish not-found from other
failures by using the store's sentinel error (e.g. errors.Is(err,
store.ErrNotFound) or the token store's specific NotFound error) after calling
tokenStore.Revoke(r.Context(), id). If the error is the not-found sentinel, keep
returning http.StatusNotFound; for any other error, log it (using
log.Error().Err(err).Str("token_id", id).Msg(...)) and return
http.StatusInternalServerError (500) with an appropriate message. Ensure you
reference tokenStore.Revoke, the error variable from that call, and the
http.Error(w, ...) response paths when making the change.
In `@internal/worker/handlers_tags.go`:
- Around line 156-174: The handler currently ignores the documented "offset"
query parameter causing pagination to always return the first page; parse
r.URL.Query().Get("offset"), convert to an int (default 0), validate it is >= 0
(and apply any project-specific max if desired), and set that value into the
search parameters before calling the search manager (assign to
search.SearchParams.Offset). Update the parsing logic near the existing limit
handling (where limit is set and searchParams is built) so the offset variable
is used when constructing searchParams in handlers_tags.go.
In `@ui/src/views/LoginView.vue`:
- Around line 62-76: Add proper ARIA linking between the token input and the
error message: on the input with id "token-input" and v-model "token" bind
aria-invalid to the error presence (e.g., :aria-invalid="Boolean(error)") and
set aria-errormessage to the error element id (e.g., :aria-errormessage="error ?
'token-error' : undefined"); change the error <p v-if="error"> to have a stable
id "token-error" and include role="alert" and aria-live="assertive" so screen
readers announce the message (ensure the id exists only when showing the error
or adjust to v-show if you need the id present while hidden).
In `@ui/src/views/VaultView.vue`:
- Line 3: The vault icon element (<i class="fas fa-vault text-4xl mb-4 block"
/>) is decorative and should be hidden from screen readers; update the element
to include accessibility attributes (add aria-hidden="true" and
focusable="false" if supported) so it is ignored by assistive technologies,
ensuring any meaningful text remains available elsewhere in the component (e.g.,
keep visible heading or label elements like VaultView title).
---
Nitpick comments:
In `@internal/db/gorm/token_store.go`:
- Around line 49-60: FindByPrefix in TokenStore returns the first active row for
an 8-char prefix which will eventually collide with the short generator in
internal/worker/handlers_auth.go; change the lookup to use a longer, uniquely
indexed search key instead of the 8-char prefix: add/use a dedicated unique
searchable column on APIToken (e.g., token_key or full_token_id) with a DB
unique/index constraint and update TokenStore.FindByPrefix to query that field
(or require a longer prefix length consistent with the generator), and ensure
the token generator produces and stores that longer unique key so bcrypt
comparisons always target the correct token record.
In `@internal/worker/middleware.go`:
- Around line 253-257: Middleware currently maps any valid session cookie to the
"admin" role, causing a fail-open; update the logic in Middleware where it reads
the cookie (see authenticateSessionCookie and authRoleKey usage) so that
authenticateSessionCookie returns the authenticated role (or additional
metadata) instead of a boolean, and then set ctx with that returned role only if
it is explicitly allowed (e.g., equals "admin") — alternatively, if you keep
authenticateSessionCookie as-is, fetch and validate the role stored inside the
cookie payload after signature/expiry verification and reject cookies that do
not assert "admin" rather than always writing "admin" into context; apply the
same change to the other Middleware block referenced (lines ~326-359).
In `@ui/src/views/AnalyticsView.vue`:
- Around line 2-6: Extract the repeated placeholder markup into a reusable Vue
component (e.g., EmptyPlaceholder or PlaceholderView) that accepts props for
icon (prop name: iconClass), title (prop name: title), and description (prop
name: description), implement the template using the existing structure and
classes, export it, and then replace the duplicated block in AnalyticsView.vue
by importing and using <EmptyPlaceholder :iconClass="'fas fa-chart-line'"
:title="'Analytics'" :description="'Usage analytics and trends. Coming in Phase
11.'"/>; update other views that use the same markup to consume this new
component as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3860f09-5dbc-4361-acf5-0e49f442db29
📒 Files selected for processing (37)
internal/db/gorm/migrations.gointernal/db/gorm/models.gointernal/db/gorm/token_store.gointernal/worker/handlers_analytics.gointernal/worker/handlers_auth.gointernal/worker/handlers_maintenance.gointernal/worker/handlers_sessions_rest.gointernal/worker/handlers_tags.gointernal/worker/handlers_vault.gointernal/worker/middleware.gointernal/worker/service.gointernal/worker/token_stats.goui/package.jsonui/src/App.vueui/src/components/layout/AppHeader.vueui/src/components/layout/AppSidebar.vueui/src/components/layout/ConfirmDialog.vueui/src/components/layout/EmptyState.vueui/src/components/layout/Pagination.vueui/src/composables/index.tsui/src/composables/useAuth.tsui/src/main.tsui/src/router/index.tsui/src/views/AnalyticsView.vueui/src/views/GraphView.vueui/src/views/HomeView.vueui/src/views/LoginView.vueui/src/views/LogsView.vueui/src/views/ObservationDetailView.vueui/src/views/ObservationsView.vueui/src/views/PatternsView.vueui/src/views/SearchView.vueui/src/views/SessionsView.vueui/src/views/SystemView.vueui/src/views/TokensView.vueui/src/views/VaultView.vueui/tailwind.config.js
Comment on lines
+1217
to
+1230
| `CREATE TABLE IF NOT EXISTS api_tokens ( | ||
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), | ||
| name TEXT NOT NULL UNIQUE, | ||
| token_hash TEXT NOT NULL, | ||
| token_prefix TEXT NOT NULL, | ||
| scope TEXT NOT NULL DEFAULT 'read-write', | ||
| created_at TIMESTAMPTZ NOT NULL DEFAULT now(), | ||
| last_used_at TIMESTAMPTZ, | ||
| request_count BIGINT NOT NULL DEFAULT 0, | ||
| error_count BIGINT NOT NULL DEFAULT 0, | ||
| revoked BOOLEAN NOT NULL DEFAULT false, | ||
| revoked_at TIMESTAMPTZ | ||
| )`, | ||
| `CREATE INDEX IF NOT EXISTS idx_api_tokens_prefix ON api_tokens (token_prefix) WHERE NOT revoked`, |
There was a problem hiding this comment.
token_prefix нужно защитить уникальным partial index.
Аутентификация ищет токен по префиксу, а схема сейчас допускает два активных ряда с одинаковым token_prefix. В такой ситуации lookup становится неоднозначным, и валидный токен может начать отвергаться после появления коллизии.
💡 Вариант правки
- `CREATE INDEX IF NOT EXISTS idx_api_tokens_prefix ON api_tokens (token_prefix) WHERE NOT revoked`,
+ `CREATE UNIQUE INDEX IF NOT EXISTS idx_api_tokens_prefix ON api_tokens (token_prefix) WHERE NOT revoked`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| `CREATE TABLE IF NOT EXISTS api_tokens ( | |
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), | |
| name TEXT NOT NULL UNIQUE, | |
| token_hash TEXT NOT NULL, | |
| token_prefix TEXT NOT NULL, | |
| scope TEXT NOT NULL DEFAULT 'read-write', | |
| created_at TIMESTAMPTZ NOT NULL DEFAULT now(), | |
| last_used_at TIMESTAMPTZ, | |
| request_count BIGINT NOT NULL DEFAULT 0, | |
| error_count BIGINT NOT NULL DEFAULT 0, | |
| revoked BOOLEAN NOT NULL DEFAULT false, | |
| revoked_at TIMESTAMPTZ | |
| )`, | |
| `CREATE INDEX IF NOT EXISTS idx_api_tokens_prefix ON api_tokens (token_prefix) WHERE NOT revoked`, | |
| `CREATE TABLE IF NOT EXISTS api_tokens ( | |
| id UUID PRIMARY KEY DEFAULT gen_random_uuid(), | |
| name TEXT NOT NULL UNIQUE, | |
| token_hash TEXT NOT NULL, | |
| token_prefix TEXT NOT NULL, | |
| scope TEXT NOT NULL DEFAULT 'read-write', | |
| created_at TIMESTAMPTZ NOT NULL DEFAULT now(), | |
| last_used_at TIMESTAMPTZ, | |
| request_count BIGINT NOT NULL DEFAULT 0, | |
| error_count BIGINT NOT NULL DEFAULT 0, | |
| revoked BOOLEAN NOT NULL DEFAULT false, | |
| revoked_at TIMESTAMPTZ | |
| )`, | |
| `CREATE UNIQUE INDEX IF NOT EXISTS idx_api_tokens_prefix ON api_tokens (token_prefix) WHERE NOT revoked`, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/db/gorm/migrations.go` around lines 1217 - 1230, The api_tokens
table allows duplicate active token_prefix values because the current non-unique
index idx_api_tokens_prefix doesn't enforce uniqueness; change the migration to
create a unique partial index on token_prefix for non-revoked rows (e.g.,
replace `CREATE INDEX IF NOT EXISTS idx_api_tokens_prefix ...` with `CREATE
UNIQUE INDEX IF NOT EXISTS idx_api_tokens_prefix_unique ON api_tokens
(token_prefix) WHERE NOT revoked`) so lookups by prefix are unambiguous; ensure
you remove or replace the old non-unique index entry in the migration statements
(in the block that defines the api_tokens table/indexes in migrations.go) to
avoid conflicting indexes.
Comment on lines
+64
to
+76
| func (s *TokenStore) Revoke(ctx context.Context, id string) error { | ||
| now := time.Now() | ||
| result := s.db.WithContext(ctx). | ||
| Model(&APIToken{}). | ||
| Where("id = ?", id). | ||
| Updates(map[string]interface{}{ | ||
| "revoked": true, | ||
| "revoked_at": &now, | ||
| }) | ||
| if result.RowsAffected == 0 { | ||
| return gorm.ErrRecordNotFound | ||
| } | ||
| return result.Error |
There was a problem hiding this comment.
Сначала обрабатывайте ошибку обновления, потом проверяйте, затронута ли запись.
Сейчас проверка затронутых строк идёт первой, поэтому сбой UPDATE легко замаскировать как отсутствие записи. Из-за этого верхний слой может ответить 404 вместо 500.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/db/gorm/token_store.go` around lines 64 - 76, The Revoke method
currently checks result.RowsAffected before inspecting the update error, which
can mask DB errors as a not-found; change TokenStore.Revoke (the call starting
with s.db.WithContext(ctx).Model(&APIToken{}).Where(...).Updates(...)) to first
check result.Error and return it if non-nil, and only after a nil error check
result.RowsAffected and return gorm.ErrRecordNotFound when RowsAffected == 0;
keep the "revoked" and "revoked_at" Updates logic unchanged.
Comment on lines
+60
to
+62
| // Get observations for analysis (rough estimate of limit) | ||
| obs, err := s.observationStore.GetRecentObservations(r.Context(), project, days*50) | ||
| if err != nil { |
There was a problem hiding this comment.
Оценочный лимит days*50 может искажать аналитику.
Если фактический объём выше этой оценки, часть наблюдений не попадёт в расчёты, и метрики трендов будут неверными.
💡 Рекомендация
- obs, err := s.observationStore.GetRecentObservations(r.Context(), project, days*50)
+ // Лучше использовать постраничную выборку до startEpoch
+ // (или запрос с фильтром по времени на уровне store), чтобы не терять данные.
+ obs, err := s.observationStore.GetRecentObservations(r.Context(), project, days*50)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/worker/handlers_analytics.go` around lines 60 - 62, The code is
using a rough cap days*50 when calling s.observationStore.GetRecentObservations
which can truncate data and skew analytics; change the call in the analytics
handler to fetch all relevant observations (remove the days*50 cap) or implement
proper pagination/streaming: update the usage of
s.observationStore.GetRecentObservations (or replace with a new method such as
GetObservationsSince/GetAllRecentObservations) to accept no artificial small
limit or iterate pages until exhausted, ensuring the full set of observations
for the requested range is retrieved before computing trend metrics.
Comment on lines
+95
to
+102
| http.SetCookie(w, &http.Cookie{ | ||
| Name: sessionCookieName, | ||
| Value: cookieValue, | ||
| Path: "/", | ||
| MaxAge: sessionMaxAge, | ||
| HttpOnly: true, | ||
| SameSite: http.SameSiteStrictMode, | ||
| }) |
There was a problem hiding this comment.
Админскую session-cookie нельзя выдавать без Secure.
HttpOnly и SameSite здесь не мешают браузеру отправить cookie по обычному HTTP. Для продовых запросов её нужно помечать Secure и очищать с теми же атрибутами; если нужен localhost по HTTP, делайте это условно по TLS/proxy-признаку.
Also applies to: 118-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/worker/handlers_auth.go` around lines 95 - 102, The session cookie
is set without the Secure flag; update the http.SetCookie call that creates the
admin session (where sessionCookieName, cookieValue, sessionMaxAge are used) to
set Secure: true for production, and ensure the cookie-clearing path uses the
same Secure, Path, HttpOnly, SameSite attributes; detect TLS/proxy by checking
request.TLS != nil or X-Forwarded-Proto == "https" and only omit Secure for
local non-TLS development.
Comment on lines
+334
to
+338
| func getAuthRole(r *http.Request) string { | ||
| if role, ok := r.Context().Value(authRoleKey{}).(string); ok { | ||
| return role | ||
| } | ||
| return "admin" // default if middleware didn't set it (backward compat) |
There was a problem hiding this comment.
Отсутствующий role не должен фолбэчиться в admin.
Если authRoleKey не проставлен, getAuthRole() молча повышает запрос до админского. Для auth-helper безопаснее вернуть пустую роль и дать вызывающему коду явно обработать этот случай.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/worker/handlers_auth.go` around lines 334 - 338, getAuthRole
currently defaults to "admin" when authRoleKey is missing, silently elevating
privileges; change it to return an empty string instead so callers must handle
missing role explicitly. Update the getAuthRole(r *http.Request) function to
detect absence of authRoleKey{} and return "" (empty string) rather than
"admin", and ensure any callers of getAuthRole (e.g., auth-helper usage sites)
handle the empty-role case as needed.
Comment on lines
+18
to
+49
| <Teleport to="body"> | ||
| <Transition name="fade"> | ||
| <div v-if="show" class="fixed inset-0 z-50 flex items-center justify-center p-4"> | ||
| <!-- Backdrop --> | ||
| <div class="absolute inset-0 bg-black/60 backdrop-blur-sm" @click="emit('cancel')" /> | ||
|
|
||
| <!-- Dialog --> | ||
| <div class="relative glass border border-white/10 rounded-xl p-6 max-w-sm w-full shadow-2xl"> | ||
| <h3 class="text-lg font-semibold text-white mb-2">{{ title }}</h3> | ||
| <p class="text-sm text-slate-400 mb-6">{{ message }}</p> | ||
|
|
||
| <div class="flex items-center justify-end gap-3"> | ||
| <button | ||
| class="px-4 py-2 rounded-lg text-sm text-slate-400 hover:text-white hover:bg-slate-800/50 transition-colors" | ||
| @click="emit('cancel')" | ||
| > | ||
| {{ cancelLabel ?? 'Cancel' }} | ||
| </button> | ||
| <button | ||
| :class="[ | ||
| 'px-4 py-2 rounded-lg text-sm font-medium transition-colors', | ||
| danger | ||
| ? 'bg-red-500/20 text-red-400 hover:bg-red-500/30 border border-red-500/50' | ||
| : 'bg-claude-500 text-white hover:bg-claude-400', | ||
| ]" | ||
| @click="emit('confirm')" | ||
| > | ||
| {{ confirmLabel ?? 'Confirm' }} | ||
| </button> | ||
| </div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Сделайте модалку полноценным диалогом для клавиатуры и скринридеров.
Сейчас окно рендерится как обычный div: нет role="dialog"/aria-modal, фокус не переводится внутрь и Esc не закрывает его. Для общего ConfirmDialog это ломает навигацию с клавиатуры и позволяет таб-фокусу уходить под оверлей.
Comment on lines
+14
to
+18
| const totalPages = computed(() => Math.max(1, Math.ceil(props.total / props.limit))) | ||
| const currentPage = computed(() => Math.floor(props.offset / props.limit) + 1) | ||
|
|
||
| const showingFrom = computed(() => (props.total === 0 ? 0 : props.offset + 1)) | ||
| const showingTo = computed(() => Math.min(props.offset + props.limit, props.total)) |
There was a problem hiding this comment.
Защитите компонент от limit <= 0 и устаревшего offset.
При limit <= 0 totalPages/currentPage становятся Infinity, а цикл в visiblePages уже не завершится, потому что Infinity++ остаётся Infinity. Даже при валидном page size здесь всё ещё можно получить некорректный диапазон из-за отрицательного или слишком большого offset, поэтому оба значения лучше нормализовать до вычислений.
Also applies to: 21-50, 53-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/components/layout/Pagination.vue` around lines 14 - 18, Normalize and
clamp props.limit and props.offset before deriving pagination values: create
normalizedLimit = Math.max(1, Math.floor(props.limit || 1)) and normalizedOffset
= Math.max(0, Math.min(Math.floor(props.offset || 0), Math.max(0, props.total -
normalizedLimit))); then use normalizedLimit/normalizedOffset in the
computations for totalPages, currentPage, showingFrom, showingTo and any
visiblePages logic so you never get Infinity or out-of-range pages; update
references to use those normalized identifiers (totalPages, currentPage,
showingFrom, showingTo, visiblePages) throughout the component.
Comment on lines
+69
to
+75
| <button | ||
| class="px-2 py-1 rounded text-slate-400 hover:text-white hover:bg-slate-800/50 transition-colors disabled:opacity-30 disabled:cursor-not-allowed" | ||
| :disabled="currentPage <= 1" | ||
| @click="goToPage(currentPage - 1)" | ||
| > | ||
| <i class="fas fa-chevron-left text-xs" /> | ||
| </button> |
There was a problem hiding this comment.
У кнопок пагинации не хватает семантики кнопки.
Без type="button" этот компонент начнёт сабмитить родительский <form> при встраивании в фильтры/поиск. Плюс у icon-only кнопок Previous/Next сейчас нет доступного имени.
Предлагаемое исправление
<button
+ type="button"
+ aria-label="Previous page"
class="px-2 py-1 rounded text-slate-400 hover:text-white hover:bg-slate-800/50 transition-colors disabled:opacity-30 disabled:cursor-not-allowed"
:disabled="currentPage <= 1"
`@click`="goToPage(currentPage - 1)"
>
- <i class="fas fa-chevron-left text-xs" />
+ <i class="fas fa-chevron-left text-xs" aria-hidden="true" />
</button>
@@
<button
v-else
+ type="button"
+ :aria-current="page === currentPage ? 'page' : undefined"
:class="[
'px-2.5 py-1 rounded transition-colors',
page === currentPage
? 'bg-claude-500/20 text-claude-400 font-medium'
: 'text-slate-400 hover:text-white hover:bg-slate-800/50',
]"
`@click`="goToPage(page)"
>
@@
<button
+ type="button"
+ aria-label="Next page"
class="px-2 py-1 rounded text-slate-400 hover:text-white hover:bg-slate-800/50 transition-colors disabled:opacity-30 disabled:cursor-not-allowed"
:disabled="currentPage >= totalPages"
`@click`="goToPage(currentPage + 1)"
>
- <i class="fas fa-chevron-right text-xs" />
+ <i class="fas fa-chevron-right text-xs" aria-hidden="true" />
</button>Also applies to: 80-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/components/layout/Pagination.vue` around lines 69 - 75, Add proper
button semantics to the Pagination.vue icon-only controls: set type="button" on
the Previous and Next <button> elements to prevent accidental form submission,
and provide accessible names (e.g., aria-label="Previous page" and
aria-label="Next page" or include visually-hidden text) so the icon-only buttons
are announced by screen readers; update the buttons that call
goToPage(currentPage - 1) and goToPage(currentPage + 1) (and any similar page
buttons between lines 80-101) accordingly.
Comment on lines
+20
to
+27
| async function login(token: string): Promise<boolean> { | ||
| const res = await fetch('/api/auth/login', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ token }), | ||
| }) | ||
| authenticated.value = res.ok | ||
| return res.ok |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "useAuth.ts" -type fRepository: thebtf/engram
Length of output: 88
🏁 Script executed:
cat -n ui/src/composables/useAuth.tsRepository: thebtf/engram
Length of output: 1427
login() может вызвать необработанное исключение вместо возврата false.
fetch() выбрасывает исключение на сетевых ошибках, но в login() это не обработано. Это нарушает контракт функции, обещающей Promise<boolean>. В отличие от этого, checkAuth() корректно обрабатывает такие ошибки с помощью try/catch.
Предлагаемое исправление
async function login(token: string): Promise<boolean> {
- const res = await fetch('/api/auth/login', {
- method: 'POST',
- headers: { 'Content-Type': 'application/json' },
- body: JSON.stringify({ token }),
- })
- authenticated.value = res.ok
- return res.ok
+ try {
+ const res = await fetch('/api/auth/login', {
+ method: 'POST',
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify({ token }),
+ })
+ authenticated.value = res.ok
+ return res.ok
+ } catch {
+ authenticated.value = false
+ return false
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function login(token: string): Promise<boolean> { | |
| const res = await fetch('/api/auth/login', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ token }), | |
| }) | |
| authenticated.value = res.ok | |
| return res.ok | |
| async function login(token: string): Promise<boolean> { | |
| try { | |
| const res = await fetch('/api/auth/login', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ token }), | |
| }) | |
| authenticated.value = res.ok | |
| return res.ok | |
| } catch { | |
| authenticated.value = false | |
| return false | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/composables/useAuth.ts` around lines 20 - 27, The login() function can
throw on network errors because fetch is unhandled; wrap the fetch call in a
try/catch (similar to checkAuth()) so any exceptions set authenticated.value =
false and the function returns false; on error optionally log the error for
debugging and only set authenticated.value = true when res.ok is true.
Comment on lines
+79
to
+87
| router.beforeEach(async (to) => { | ||
| const { authenticated, loading, checkAuth } = useAuth() | ||
|
|
||
| if (loading.value) { | ||
| await checkAuth() | ||
| } | ||
|
|
||
| if (!to.meta.public && !authenticated.value) { | ||
| return { name: 'login' } |
There was a problem hiding this comment.
❓ Verification inconclusive
Script executed:
find . -type f -name "index.ts" -o -name "router.ts" | grep -E "router|route" | head -20Repository: thebtf/engram
Repository: thebtf/engram
Exit code: 0
stdout:
./ui/src/router/index.ts
Script executed:
fd -t f "index.ts" ui/src/routerRepository: thebtf/engram
Repository: thebtf/engram
Exit code: 0
stdout:
ui/src/router/index.ts
Script executed:
cat ui/src/router/index.ts 2>/dev/null | head -100Repository: thebtf/engram
Repository: thebtf/engram
Exit code: 0
stdout:
import { createRouter, createWebHashHistory } from 'vue-router'
import { useAuth } from '@/composables/useAuth'
const routes = [
{
path: '/login',
name: 'login',
component: () => import('@/views/LoginView.vue'),
meta: { public: true },
},
{
path: '/',
name: 'home',
component: () => import('@/views/HomeView.vue'),
},
{
path: '/observations',
name: 'observations',
component: () => import('@/views/ObservationsView.vue'),
},
{
path: '/observations/:id',
name: 'observation-detail',
component: () => import('@/views/ObservationDetailView.vue'),
},
{
path: '/search',
name: 'search',
component: () => import('@/views/SearchView.vue'),
},
{
path: '/vault',
name: 'vault',
component: () => import('@/views/VaultView.vue'),
},
{
path: '/logs',
name: 'logs',
component: () => import('@/views/LogsView.vue'),
},
{
path: '/graph',
name: 'graph',
component: () => import('@/views/GraphView.vue'),
},
{
path: '/patterns',
name: 'patterns',
component: () => import('@/views/PatternsView.vue'),
},
{
path: '/sessions',
name: 'sessions',
component: () => import('@/views/SessionsView.vue'),
},
{
path: '/analytics',
name: 'analytics',
component: () => import('@/views/AnalyticsView.vue'),
},
{
path: '/system',
name: 'system',
component: () => import('@/views/SystemView.vue'),
},
{
path: '/tokens',
name: 'tokens',
component: () => import('@/views/TokensView.vue'),
},
]
const router = createRouter({
history: createWebHashHistory(),
routes,
})
// Navigation guard: redirect to login when not authenticated
router.beforeEach(async (to) => {
const { authenticated, loading, checkAuth } = useAuth()
if (loading.value) {
await checkAuth()
}
if (!to.meta.public && !authenticated.value) {
return { name: 'login' }
}
if (to.name === 'login' && authenticated.value) {
return { name: 'home' }
}
})
export default router
Script executed:
fd -t f "useAuth" ui/src/Repository: thebtf/engram
Repository: thebtf/engram
Exit code: 0
stdout:
ui/src/composables/useAuth.ts
Script executed:
find . -type f -name "*useAuth*" -o -name "*auth*" | grep -E "composable|hook" | head -20Repository: thebtf/engram
Repository: thebtf/engram
Exit code: 0
stdout:
./ui/src/composables/useAuth.ts
Script executed:
find ui/src -type f \( -name "*.ts" -o -name "*.js" -o -name "*.vue" \) | xargs grep -l "useAuth\|composables" | head -20Repository: thebtf/engram
Repository: thebtf/engram
Exit code: 0
stdout:
ui/src/App.vue
ui/src/components/FilterTabs.vue
ui/src/components/Header.vue
ui/src/components/Sidebar.vue
ui/src/components/layout/AppHeader.vue
ui/src/components/layout/AppSidebar.vue
ui/src/composables/index.ts
ui/src/composables/useAuth.ts
ui/src/router/index.ts
ui/src/views/HomeView.vue
ui/src/views/LoginView.vue
Script executed:
cat ui/src/composables/useAuth.tsRepository: thebtf/engram
Repository: thebtf/engram
Exit code: 0
stdout:
import { ref, computed } from 'vue'
// Singleton state shared across all useAuth() calls
const authenticated = ref(false)
const loading = ref(true)
export function useAuth() {
async function checkAuth(): Promise<void> {
loading.value = true
try {
const res = await fetch('/api/auth/me')
authenticated.value = res.ok
} catch {
authenticated.value = false
} finally {
loading.value = false
}
}
async function login(token: string): Promise<boolean> {
const res = await fetch('/api/auth/login', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ token }),
})
authenticated.value = res.ok
return res.ok
}
async function logout(): Promise<void> {
try {
await fetch('/api/auth/logout', { method: 'POST' })
} finally {
authenticated.value = false
}
}
return {
authenticated: computed(() => authenticated.value),
loading: computed(() => loading.value),
checkAuth,
login,
logout,
}
}
Guard не переповторяет проверку авторизации при повторных переходах, что может привести к false positive блокировкам.
На первом переходе loading.value инициализируется как true, поэтому checkAuth() вызывается корректно. Однако после завершения проверки loading становится false. При следующих переходах на защищённые маршруты, если пользователь всё ещё не аутентифицирован, checkAuth() больше не вызывается, и состояние авторизации не обновляется, даже если сессия могла восстановиться.
💡 Предлагаемое исправление
router.beforeEach(async (to) => {
const { authenticated, loading, checkAuth } = useAuth()
- if (loading.value) {
+ if (loading.value || !authenticated.value && !to.meta.public) {
await checkAuth()
}
if (!to.meta.public && !authenticated.value) {
return { name: 'login' }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/router/index.ts` around lines 79 - 87, The route guard in
router.beforeEach uses useAuth() but only calls checkAuth() when loading.value
is true, so subsequent navigations to protected routes can skip re-checking and
cause false positives; change the logic so that for non-public routes
(to.meta.public falsy) you always await checkAuth() when either loading.value is
true OR authenticated.value is false, then re-evaluate authenticated.value and
return { name: 'login' } only if it remains false; update the guard around
router.beforeEach, useAuth, loading.value, checkAuth and authenticated.value
accordingly so the auth state is refreshed before blocking navigation.
This was referenced Mar 19, 2026
This was referenced Mar 19, 2026
This was referenced Apr 14, 2026
This was referenced Apr 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Frontend foundation for Engram Dashboard (Phase 3):
Depends on: PR #18 (REST endpoints), PR #17 (auth subsystem)
Test plan
cd ui && npm install && npm run buildsucceedsSummary by CodeRabbit
Примечания к выпуску
Новые функции
Стили