fix(server,web): PR #786 follow-up — pi review findings + dead-code sweep#803
Closed
buremba wants to merge 11 commits into
Closed
fix(server,web): PR #786 follow-up — pi review findings + dead-code sweep#803buremba wants to merge 11 commits into
buremba wants to merge 11 commits into
Conversation
Bump packages/web to feat/loading-skeletons (210afd3). See the submodule commit for full detail. DESIGN_GUIDELINES §5 calls for skeleton placeholders on content panels; the codebase had drifted to 9 page-level panels showing bare "Loading…" text and a few pages flashing it twice in sequence from unsynced queries. This swap adds a <Skeleton> primitive, upgrades TabLoadingState to render shimmer rows / cards / panels, and rewires every page-level violation. Also includes a small connector-sidebar filter tweak and the OrgRootRedirect synchronous-Navigate fix that were sitting in the web working tree.
…eton Bump packages/web to 2237b6e — see the submodule commit for full detail. Wires TanStack Router loaders + a router-level pending skeleton so navigation lands on warm cache and only true cold loads flash the placeholder. - Hook factories expose queryOptions(...) so loaders can call ensureQueryData without duplicating queryKey/queryFn. - 7 routes (entity-list, agents, clients, connector index, events, members, oauth-apps) prefetch their primary data; defaultPreload 'intent' warms them on link hover. - Router gets defaultPendingComponent (TabLoadingState) and defaultPendingMs:150 — cached navigation feels instant; only true cold loads see the skeleton.
Bump packages/web to 52d4463. See submodule commit for detail. - 4 more routes get loaders: agent detail, connection detail, device detail, watcher redirect (via beforeLoad). - useDevices + useMetricSeries get queryOptions companions so the agents-index / events-index loaders can prefetch sparkline data alongside primary list data — StatsStrip no longer paints in two waves. - Device detail's "Loading device…" / FeedsSidebar "Loading…" are now skeletons. - Watcher redirect moves from useEffect+navigate to throw redirect() in beforeLoad, so the old route never mounts and the redirect becomes a single navigation.
Bump packages/web to f3a7355 — drops the redirect-only watcher route, the unused feed-expanded-row.tsx (450 LOC), useWorkspaceBootstrap, and useDeleteNotification. OwnerTabPage loading block now uses the shared <Skeleton> primitive.
…er orphan archive
Bundled with the loading-skeletons PR per scope discussion. Three
backend tweaks the FE pass leaned on or surfaced, plus a one-shot data
cleanup.
manage_entity.list — server-side linked-column resolution
- New `linked_entities` field on the list response: keyed by
`${entityType}:${lookupField}` then by the raw metadata value. The FE
used to fan out one manage_entity.list per `x-link-entity-type`
column (~2.5s × 4 columns on the Company page); the loader now does
it inline in one parallel Promise.all alongside relationship batches.
manage_feeds.list — collapse correlated event_count subquery
- The previous shape ran
`SELECT COUNT(*) FROM current_event_records WHERE connection_id=…
AND feed_key=…` per feed row. On busy connections that was ~880ms
per row × N feeds — a multi-second feed list.
- Now uses a MATERIALIZED page CTE + a single GROUP BY over
`(connection_id, feed_key)`, scoped by
`connection_id = ANY(ARRAY(SELECT DISTINCT connection_id FROM page))`
to keep the planner on one index scan instead of repeating it per
pair.
manage_watchers.create — reject zombie watchers
- The scheduler at `packages/server/src/watchers/automation.ts:469`
filters with `WHERE w.agent_id IS NOT NULL`, so a watcher without an
agent never runs. The argument schema marks `agent_id` optional
(shared across create/update/delete actions) but create now throws a
`ToolUserError` when it's missing.
Migration 20260517040000_archive_orphan_watchers
- One-shot data cleanup: flips `status='active' AND agent_id IS NULL`
rows to `archived`. Prod audit 2026-05-17 found 28 such rows across
11 orgs — all originated from older create paths before agent_id was
wired in. Migration is up-only (no-op down); the create-guard above
prevents the orphan set from regrowing.
Submodule (packages/web): paired refactor of the entity-type table to
consume `linked_entities` instead of fanning out useQueries lookups.
Bump packages/web to 3b76016. ~30 unused exports either unexported (when only used in-file) or deleted (dead). Shadcn primitive re-exports left intact as surface area.
Bump packages/web to 9eafd4e. Fixes:
1. Entity-list loader was prefetching the wrong cache entry — component
passed full useOrgContext() with organizationId, loader passed only
{ slug }. Loader now chains workspace fetch and mirrors component
list options.
2. Restore /$owner/watchers/$watcherId — without it, old deep links
silently land on /connectors. Brought back as a beforeLoad redirect.
3. Wrap chained workspace fetch in try/catch; switch secondary
prefetches to prefetchQuery so component error states still drive
the page UX instead of the global route-error.
4. StatusDot tone=null no longer returns null (caused layout shift in
sidebar rows mixing healthy/unhealthy items) — transparent
placeholder of the same size, aria-hidden.
pi flagged that handleCreate in manage_watchers rejected agent_id=null but handleUpdate didn't — so the active-orphan-watcher state could be reproduced after the archive migration. Two layers of defense now: - DB: new migration 20260517050000 deletes the residual agent_id IS NULL rows and sets `watchers.agent_id NOT NULL`. The existing partial index (predicate `WHERE agent_id IS NOT NULL`) becomes a tautology once the column is NOT NULL, so it's replaced with an unconditional index. - App: handleUpdate now rejects explicit nulling and rejects scheduling a watcher with no owning agent — fails earlier with a clear error message instead of letting the constraint violation surface raw. Also bundles a small manage_entity tweak to route a text-array bind through the new pgTextArray helper (consistent with the rest of the linked-columns resolver).
…etons # Conflicts: # packages/web
…owing, submodule bump - db/schema.sql: relocate idx_personal_access_tokens_worker_id to its alphabetical position and add the missing COLUMN COMMENT so the file matches `dbmate up` output (the migrations CI job was failing on drift). - manage_feeds.list_feeds: add `feed_key = ANY(ARRAY(...))` alongside the connection-id ANY so the events scan narrows on both axes; cuts prod 50-feed list from ~1.5s to ~670ms. - Bump packages/web to 3e67f098 (pickSeries hardening + connection detail useFeeds prefetch).
📝 WalkthroughWalkthroughEnforces non-null ChangesWatcher Agent ID NOT NULL Enforcement
Admin Tool Enhancements
Dependency Update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b46995f16
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const linkedType = (prop as { 'x-link-entity-type'?: unknown })['x-link-entity-type']; | ||
| if (typeof linkedType !== 'string' || linkedType === '') continue; | ||
| const lookupFieldRaw = (prop as { 'x-link-lookup-field'?: unknown })['x-link-lookup-field']; | ||
| const lookupField = typeof lookupFieldRaw === 'string' && lookupFieldRaw ? lookupFieldRaw : 'slug'; |
There was a problem hiding this comment.
When a schema declares x-link-entity-type without x-link-lookup-field, this defaults to slug, but several built-in schemas use integer FK columns without an explicit lookup field (for example examples/atlas/models/schema.yaml:11-16 and examples/market/models/schema.yaml:303-308). For those pages the visible metadata values are entity IDs like company_id, so this new server-side resolver queries e.slug = ANY(...) with numeric IDs and returns no linked_entities, regressing the inline linked-entity display that this path is meant to replace.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/server/src/tools/admin/manage_watchers.ts`:
- Around line 931-940: The guard that checks args.agent_id currently only tests
truthiness and allows whitespace-only IDs; change the validation in the
create-paths of manage_watchers so agent_id is normalized and rejected if empty
after trimming (e.g., if typeof args.agent_id !== 'string' or
args.agent_id.trim().length === 0 then throw the same ToolUserError). Update the
checks referencing args.agent_id in the create branch (the block with
ToolUserError), and replicate the same trimmed-empty validation at the other
occurrences noted (the checks around lines 1143-1149 and 1238-1257) so
whitespace-only IDs cannot be stored.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 80d6990b-fb43-474a-a776-f5603171dc5b
📒 Files selected for processing (7)
db/migrations/20260517040000_archive_orphan_watchers.sqldb/migrations/20260517050000_watcher_agent_id_not_null.sqldb/schema.sqlpackages/server/src/tools/admin/manage_entity.tspackages/server/src/tools/admin/manage_feeds.tspackages/server/src/tools/admin/manage_watchers.tspackages/web
Comment on lines
+931
to
+940
| if (!args.agent_id) { | ||
| // The scheduler joins on `agent_id IS NOT NULL` (see | ||
| // packages/server/src/watchers/automation.ts:469), so a watcher without | ||
| // an agent has no way to execute. Schema-wise `agent_id` is `Type.Optional` | ||
| // because the field is shared across all manage_watchers actions, but | ||
| // create enforces it: a watcher with no owning agent is a zombie row. | ||
| throw new ToolUserError( | ||
| 'agent_id is required to create a watcher (the agent that executes it).' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Enforce non-empty agent_id, not just non-null/truthy.
These guards still allow whitespace-only IDs (e.g. ' '). That passes DB NOT NULL but breaks the “owning agent” invariant and can still create effectively orphaned watchers.
🔧 Suggested fix
+function normalizeAgentId(value: unknown): string | null {
+ if (typeof value !== 'string') return null;
+ const trimmed = value.trim();
+ return trimmed.length > 0 ? trimmed : null;
+}
+
async function handleCreate(
@@
- if (!args.agent_id) {
+ const createAgentId = normalizeAgentId(args.agent_id);
+ if (!createAgentId) {
@@
- ${args.agent_id ?? null}, ${args.scheduler_client_id ?? null},
+ ${createAgentId}, ${args.scheduler_client_id ?? null},
@@
async function handleCreateFromVersion(
@@
- if (!version.agent_id) {
+ const sourceAgentId = normalizeAgentId(version.agent_id);
+ if (!sourceAgentId) {
@@
- ${version.agent_id ?? null}, ${version.scheduler_client_id ?? null},
+ ${sourceAgentId}, ${version.scheduler_client_id ?? null},
@@
async function handleUpdate(
@@
- if (args.agent_id === null) {
+ const updateAgentId =
+ args.agent_id === undefined ? undefined : normalizeAgentId(args.agent_id);
+ if (args.agent_id !== undefined && updateAgentId === null) {
throw new ToolUserError(
- 'agent_id cannot be set to null — every watcher must have an owning agent.'
+ 'agent_id must be a non-empty string — every watcher must have an owning agent.'
);
}
@@
- agent_id = CASE WHEN ${args.agent_id !== undefined} THEN ${args.agent_id ?? null} ELSE agent_id END,
+ agent_id = CASE WHEN ${updateAgentId !== undefined} THEN ${updateAgentId} ELSE agent_id END,Also applies to: 1143-1149, 1238-1257
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/server/src/tools/admin/manage_watchers.ts` around lines 931 - 940,
The guard that checks args.agent_id currently only tests truthiness and allows
whitespace-only IDs; change the validation in the create-paths of
manage_watchers so agent_id is normalized and rejected if empty after trimming
(e.g., if typeof args.agent_id !== 'string' or args.agent_id.trim().length === 0
then throw the same ToolUserError). Update the checks referencing args.agent_id
in the create branch (the block with ToolUserError), and replicate the same
trimmed-empty validation at the other occurrences noted (the checks around lines
1143-1149 and 1238-1257) so whitespace-only IDs cannot be stored.
buremba
added a commit
that referenced
this pull request
May 17, 2026
Member
Author
|
Superseded by #804 (cleaner branch, same scope). |
buremba
added a commit
that referenced
this pull request
May 17, 2026
…ump (#804) * fix: schema.sql drift + manage_feeds feed_key narrowing + submodule bump Three follow-ups dropped from PR #786 during the squash-merge: - db/schema.sql: insert the COLUMN COMMENT on personal_access_tokens.worker_id and move idx_personal_access_tokens_worker_id into its alphabetical spot so the file matches `dbmate up` output. The migrations CI job has been failing on this drift since 20260517030000_pat_worker_id_binding was added. - manage_feeds.list_feeds: narrow the event-counts scan by feed_key as well as connection_id (ANY-array on both axes); the LEFT JOIN at the end drops any over-count from the cross product. Cuts the 50-feed list on a busy connection from ~1.5s to ~670ms in prod. - Bump packages/web to ca12cd2 (owletto/main after PR #141), so the parent points at a submodule SHA reachable from owletto/main rather than at the now-orphaned pre-squash 2d2f5bf. * fix(schema): drop stray blank line so file matches dbmate up output * chore(submodule): bump web for useFeeds prefetch gate (pi review of #803) * test(watchers): pass agent_id on watchers.create across integration tests
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
Follow-up to #786. Three things that landed after the squash-merge:
manage_feeds.list— narrow theevent_countsCTE byfeed_key(packages/server/src/tools/admin/manage_feeds.ts).The CTE shipped in feat(web): swap bare Loading… text for skeleton placeholders #786 filtered
eventsby page connection IDs only, then GROUP BY (connection_id, feed_key). Correct, but on a connection with many off-page feeds it still scanned every feed_key. Now also filtersfeed_key = ANY(ARRAY(SELECT DISTINCT feed_key FROM page WHERE feed_key IS NOT NULL))so the scan is bounded by the actual tuples on the page.schema.sqlworker_id COMMENT placement (db/schema.sql).Local pg_dump reordered the
personal_access_tokens.worker_idCOMMENT + its partial index. No semantic change.Submodule bump (
packages/web).Pulls in the merged owletto-web work that landed after feat(web): swap bare Loading… text for skeleton placeholders #786:
/$owner/watchers/$watcherIdas a thinbeforeLoad: throw redirect()so old bookmarks resolve to the agent page instead of falling through to OwnerResolver.useFeeds.queryOptions(connectionId)so the pending skeleton covers the slowest query on cold loads.pickSerieshardening for empty results.Why two passes
#786 squashed and merged before pi flagged these. Rather than amending main, the fixes were captured on this branch and now go out as a follow-up.
Test plan
bunx tsc --noEmitinpackages/web— cleanmake build-packages— clean/<owner>/watchers/<old-watcher-id>and confirm redirect to agent pagefeedIddeep link and confirm single pending skeleton (no post-mount feed flash)Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Performance