feat(web): swap bare Loading… text for skeleton placeholders#786
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.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughServer admin tooling: list now resolves x-link-entity-type columns and batches lookups in parallel; feeds listing uses a paged CTE with grouped event counts; watcher creation requires agent_id. Adds a one-shot DB migration to archive orphan watchers, updates schema migrations, and bumps ChangesAdmin & DB changes
Submodule Update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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/web`:
- Line 1: The packages/web pointer is currently set to commit
210afd38b4342b70f4c259bd47f618906820878c which CI reports is not an ancestor of
owletto-web/main; update the pin so it references a SHA that is reachable from
origin/main (or first merge commit 210afd38... into owletto-web/main and then
update the pointer). Locate where packages/web is pinned (the commit SHA string
210afd38b4342b70f4c259bd47f618906820878c), replace it with a commit SHA that is
an ancestor of origin/main, or perform the merge and then change the pin to the
resulting reachable SHA so the CI hard drift check passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…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.
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 create-only guard for agent_id leaves a gap: the
create_from_version path can still insert active watchers with agent_id = NULL;
update the create_from_version logic in manage_watchers.ts (the
create_from_version function / code path) to enforce the same invariant as the
create branch by validating args.agent_id and throwing a ToolUserError when it's
missing, or ensure the inserted watcher rows explicitly set a non-null agent_id
(mirror the check/throw used where create enforces agent_id) so no active
watcher can be created without an owning agent.
🪄 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: b580e8ab-c21b-46f4-a124-6c36f358f67e
📒 Files selected for processing (6)
db/migrations/20260517040000_archive_orphan_watchers.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
✅ Files skipped from review due to trivial changes (1)
- packages/web
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 review appliedRan `pi -p` on the branch. Findings + resolutions: Applied:
Skipped: pi also flagged a server-side concern in `packages/server/src/tools/admin/manage_watchers.ts` — `agent_id` is guarded on create but not on update, so the "active orphan watcher" state could be re-created post-migration. That's outside this PR's scope and worth a focused follow-up. Typecheck + build clean on the head commit. |
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).
|
Pulled in the server-side fix pi flagged too:
DB + app guards layered: belt and suspenders. |
…etons # Conflicts: # packages/web
…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
2 participants
Summary
Two passes of cleanup on the perceived-loading UX:
Pass 1 — Skeleton placeholders (replace bare `Loading…`)
DESIGN_GUIDELINES §5 calls for skeleton placeholders on content panels and reserves bare `Loading…` text for sub-1s auth handoffs and tiny inline regions. The codebase had drifted: 9 page-level panels showed text-only loaders; oauth-apps stacked two unsynced queries that flashed `Loading…` twice in sequence before content appeared.
Pass 2 — Route loaders + unified pending skeleton
The deeper fix: route components were mounting first and then firing their primary `useQuery`, so users watched the page paint loading state. With `defaultPreload: 'intent'` already on, the missing piece was a loader to prefetch into the cache.
Auth-handoff loaders kept text-only
`/`, `owner-resolver`, `watchers/$watcherId` redirect, member-picker dropdown, events-tab count badge — all sub-1s or tiny inline per §5.
Also bundles small pre-existing local WIP from `packages/web` (connector sidebar filter, `OrgRootRedirect` synchronous ``, sidebar status-dot tidy, `$connectorKey.tsx` layout simplification).
Submodule: lobu-ai/owletto-web feat/loading-skeletons (commit 2237b6e).
Test plan
Summary by CodeRabbit
Chores
Bug Fixes
New Features
Refactor