Skip to content

Overhaul web search and Commander results - #427

Merged
lopugit merged 6 commits into
developfrom
codex/search-commander-overhaul
Aug 27, 2026
Merged

Overhaul web search and Commander results#427
lopugit merged 6 commits into
developfrom
codex/search-commander-overhaul

Conversation

@lopugit

@lopugit lopugit commented Aug 27, 2026

Copy link
Copy Markdown
Owner

Summary

  • search live ACL-aware Thingtime Things, posts, schemas/data, and people from Commander while keeping local paths as a bounded fallback tier
  • make Standard the default /search presentation using native PostCard and ThingView rendering
  • preserve Data mode behind an accessible toggle and add canonical /thing/:id deep links
  • show a subtle real ranked-match score in both search views when MongoDB returns one
  • make plain Enter open Commander’s pinned “Search things for …” row when no result is selected, without changing setter/action behavior
  • render an interactive PostCard inline on /thing/:id for post Things, alongside the raw Thing data
  • refresh Graphify outputs and extend the manual search regression checklist

Validation

  • focused Commander/rank-score tests (7/7)
  • corepack pnpm --dir remix run test:commander (8/8 from the initial search overhaul)
  • corepack pnpm --dir remix run typecheck:ratchet (138, down from 143 baseline)
  • targeted ESLint (0 errors; 6 existing hook warnings)
  • corepack pnpm --dir remix run build, including Vercel output verification
  • desktop 1280x720 and mobile 390x844 in-app browser QA with full-page scrolling, Standard/Data toggling, ranked/unranked searches, canonical deep links, unselected Commander Enter, inline post comments, console checks, and overflow checks
  • local /search?q=todo, post detail, and frontend health return successfully

@vercel

vercel Bot commented Aug 27, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
thingtime Ready Ready Preview Aug 27, 2026 9:29am
thingtime (develop) Ready Ready Preview Aug 27, 2026 9:29am

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Deterministic product contract advisory

These examples are warning-only. They do not fail the build/API contexts or block this PR.

  • ✅ Web CI required-context examples
  • ⚠️ Thin-listener topology examples (exit 1)
Sanitized tail
node:internal/modules/run_main:123
    triggerUncaughtException(
    ^

AssertionError [ERR_ASSERTION]: all-branch.yml must stay retired; Lopu PR manager owns its former public triggers

true !== false

    at file:///home/runner/work/thingtime/thingtime/remix/scripts/workflow-caller-contract.mjs:39:10
    at ModuleJob.run (node:internal/modules/esm/module_job:343:25)
    at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:681:26)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:117:5) {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: true,
  expected: false,
  operator: 'strictEqual',
  diff: 'simple'
}

Node.js v22.23.2

@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

🧹 Develop S3 preview removed

The PR-specific alias and every workflow-created develop deployment were removed when this PR closed.

The ordinary generated Vercel Preview remains available on the shared development runtime.

@github-actions
github-actions Bot temporarily deployed to develop-pr-427 August 27, 2026 06:48 Destroyed
@github-actions
github-actions Bot temporarily deployed to develop-pr-427 August 27, 2026 06:50 Destroyed
@github-actions

Copy link
Copy Markdown
Contributor

@/tmp/lopu-comment-427.md

@github-actions

Copy link
Copy Markdown
Contributor

Lopu repository review

Lopu reviewed this PR against develop as Thingtime's principal PR and repository manager. Using Claude Opus 5.

The PR head moved during review, so Lopu preserved the newer branch and did not overwrite it.

Lopu also opened a controller/workflow repair PR for the failed check root cause.

Lopu review — PR #427 · Overhaul web search and Commander results

Compared: codex/search-commander-overhaul@e5bcc813 against develop@25989863
(9 files; ~380 lines of real product change plus regenerated Graphify outputs).

Graphify graph.json / GRAPH_REPORT.md / manifest.json are regenerated
artifacts and were excluded from line-by-line review, per the repo convention
that they are an atomic pair refreshed by graphify update ..

Note: PR #427 advanced to e46a7fec (commits 42052bb2, e46a7fec) while
this review ran. Every finding below was re-checked against that head and all
of them still apply; the new commanderEnterSuggestionIndex helper changes
which row Enter picks but still returns a stale hoveredSuggestion verbatim.

What I checked

  • The live-search Commander tier against the real API surface: useApi
    (v1.things.search, v1.profile.search), useAsyncFetcher error semantics,
    and the things.search / users.search rate-limit config.
  • SearchPost = PublicPost against the server projection.
  • PostCard / ThingView prop contracts and the PostChange contract as
    implemented by Feed.tsx, routes/post.tsx, and routes/media.tsx.
  • Route existence for every link the PR generates (/post/:id, /thing/:id,
    /profile/:username in app/routes.tsx) and URL-encoding convention against
    the existing MigrationsPanel usage.
  • Commander keyboard/mouse selection indexing end to end.
  • The failing CodeQL check (live check-run + Actions + code-scanning APIs).
  • All 16 open CodeQL alerts on this PR head and their cited code.

Correctness / API findings

Verified sound

  • SearchPost = PublicPost is a real correction, not a risky widening.
    /api/v1/things/search returns posts: Record<string, PublicPost> built by
    toPublicPosts (api/utils/things/search.ts:396), so the old lean local type
    was under-describing the wire shape. Rendering PostCard from search results
    is therefore safe — the projection already carries attachments, comments,
    viewerReactions, shareOf, etc.
  • api.v1.profile.search exists and returns { ok, users }
    (routes/api/v1/users/search/_search.tsx:26); the Commander's
    peopleResponse?.users mirrors the pattern SearchPage already uses.
  • No toast spam on failure. useAsyncFetcher.submit throws rather than
    toasting; the Commander's catch degrades to local commands only.
  • Stale-response guarding is correct. The remoteRequestRef sequence number
    is checked before every setState, and the early-return branch bumps it, so a
    slow response for an abandoned query can never repaint.
  • Rate limiting is not a concern. things.search is 120/60s per user
    (rateLimit/config.ts:37) against a 250 ms debounce.
  • Encoding matches convention. thingDetailPath uses encodeURIComponent,
    the same as the existing /thing/:id link in MigrationsPanel.tsx:146.

Defects found and fixed in this worktree

  1. Live results hijack an active keyboard selection
    (CommanderV2.tsx). Dropdown rows are addressed by position, and the remote
    tier is inserted between row 0 and the local paths. The tier lands ~250 ms
    after the last keystroke, which is exactly when a user is arrowing. Arrow
    onto a Local paths row before the results paint and the highlight silently
    becomes the first Across Thingtime row — Enter then opens a search result
    instead of the path the user aimed at.
    Fix: shift a live hovered index by the tier's growth so the highlighted row
    keeps meaning the same thing (row 0, the pinned full-search row, never moves).

  2. Enter could become a silent no-op (CommanderV2.tsx). The PR made
    executeCommand return unconditionally after selectSuggestion(...), while
    selectSuggestion gained a bare if (!suggestion) return;. If the hovered
    row no longer resolves, Enter now does nothing at all — previously it fell
    through and still ran the typed command. (This is unchanged at e46a7fec:
    commanderEnterSuggestionIndex passes a non-null hoveredSuggestion straight
    through.)
    Fix: selectSuggestion now returns whether the row resolved; Enter
    short-circuits only when it did, so an unresolvable row falls back to running
    what the user typed. The PR's actual intent — never double-navigate after a
    real selection — is preserved exactly.

  3. The global keydown listener re-registered on every render while typing
    (CommanderV2.tsx). remoteResults was computed inline, producing a fresh
    [] whenever remoteSearch.query !== trimmedInput — i.e. throughout typing.
    That identity flows remoteResults → selectSuggestion → executeCommand → allCommanderKeyListener → useEffect, tearing down and re-adding a window
    keydown listener each render. Before this PR the equivalent input was a
    memoized value and the listener was stable.
    Fix: memoize remoteResults against a module-level empty constant.

  4. Unrelated TESTING.md reformatting — seven hunks reindented continuation
    lines from 6 to 4 spaces in sections this PR does not touch (MCP connector,
    worktree setup, branding, web-CI contexts, AI runtime contract, HTML
    no-store), and one pulled the standalone Dev bootstrap: paragraph into the
    preceding list item, changing how it renders. TESTING.md is 169 KB and this
    is pure diff noise.
    Fix: reverted those hunks; the intentional Search-page checklist addition is
    kept untouched.

Also added: a TESTING.md line covering findings 1 and 2, per the repo rule
that a fixed bug class gets a permanent checklist entry.

Reported, not changed

  • updateResultPost does not honour a functional PostChange that resolves
    to null
    (SearchPage.tsx:800). Feed.tsx:172, routes/post.tsx:78 and
    routes/media.tsx:101 all treat "function returned null" as a deletion; the
    new /search handler returns prev instead, so the card would linger.
    I confirmed PostCard only ever deletes via a direct onChanged?.(null)
    (line 1147) — every functional change it emits returns a PublicPost — so
    this is latent, not live. I left it alone rather than degrade the
    "apply the delta to the FRESHEST post" property that the type's own comment
    calls out; worth aligning deliberately.
  • presentation is not persisted to the /search local cache alongside
    rows/kind/sort, so the Standard/Data choice resets on every visit.
    Product call, not a defect.
  • total is not decremented when updateResultPost removes a deleted
    result, so the "~N things" label drifts by one until the next search.

Failing check: CodeQL — not caused by this PR

gh pr checks 427 showed one failure: the GitHub Advanced Security CodeQL
check run (98427880690), conclusion: timed_out after 16s, with the success
title "No new alerts in code changed by this pull request". Both Analyze
jobs succeeded in every run and the merge-ref analyses uploaded cleanly.

Root cause is in the protected control plane, not in this PR:
.github/workflows/codeql-analysis.yml elects one analysis owner by sampling
gh pr list once, a few seconds after the push. This branch was pushed at
06:16:34 and the PR was opened at 06:16:43 — nine seconds after the scope job
had already run (06:16:38–06:16:41) and declared the push the owner. The push
run therefore uploaded a refs/heads/… analysis for a commit that had become an
open PR head (06:17:21), GHAS opened the PR's check against that partial
branch snapshot at 06:17:20, and closed it timed_out at 06:17:36 — long before
the PR's own merge-ref analyses landed (06:17:57 and 06:22:33).

Differential evidence: PRs #426/#428 show the mirror image — their GHAS check
starts within a second of the merge-ref analysis and passes in 1–3s, and none
has a branch-ref analysis at its head; #428's push run correctly skipped because
its PR already existed. PR #427's own later commits (pushed while the PR
existed) behave the same way, so the trigger is the race, not the content.

Per the split-permission rule this fix does not ride on the PR branch. It is
staged in the protected controller checkout — one file,
.github/workflows/codeql-analysis.yml — with the full diagnosis and validation
in lopu-workflow-fix.md, for a dedicated Lopu fix PR against github-actions.

CodeQL alerts on this head

All 16 open alerts are pre-existing repository findings; none is in a file
this PR touches, and CodeQL itself reports no new alerts in the changed code.

Dispositioned as not applicable (11) — each with cited evidence:

alerts files reason
7 api/utils/userGenerateJWT.ts false positive — unimplemented stub, no importers, uuid never read, no JWT minted
8, 14, 26 LogoOld3.tsx, useUuid.tsx, Thingtime.tsx false positive — DOM ids / React keys / CSS @keyframes names
27, 28, 29 useThingtimeMachine.tsx false positivedebugUuid is assigned and never read; the timeline uuid is an in-memory undo/redo key
77–80 scripts/verify-admin-subscriptions.mjs, scripts/verify-app-storage.mjs used in tests — throwaway fixture account names/passwords in local dev-stack verification scripts

Left open — real findings, deliberately not bundled into a search-UI PR (5):

  • 18, 19app/smarts/index.tsx:1225,1247: merge() copies keys from
    value2 with no __proto__ / constructor / prototype guard. This is a
    genuine prototype-pollution-capable deep merge at the heart of Thingtime.
    It deserves its own PR with tests; a semantics change to smarts has far too
    wide a blast radius to land inside a search overhaul.
  • 45deprecated/api/src/index.js:41: origin: '*' on both the Express
    and socket.io CORS config. The deprecated/ tree is referenced by nothing —
    not vercel.json, scripts/vercel-build.mjs, any workflow, or
    ecosystem.config.js — so it is unreachable, but the pattern is real. The
    right fix is deleting the dead package, not dismissing the alert.
  • 17raycast/src/commands/regexToReplacementConverter.tsx:57: a genuine
    double-unescaping bug (\\\\\ after \\{{) in a local Raycast dev
    utility. Real correctness defect, unrelated scope.
  • 49app/api/http.ts:12: stack-trace exposure through the shared json()
    helper, sourced from api/utils/errors/adminDiagnostic.ts. Whether this is
    acceptable depends on how the admin diagnostic surface is gated; that trace
    belongs in a focused pass, not a drive-by dismissal.

I did not dismiss any of these to green a check.

Validation run

All from remix/ at the reviewed head with my changes applied:

  • corepack pnpm install --frozen-lockfile — clean.
  • corepack pnpm run test:commander8/8 pass.
  • corepack pnpm run lint:files -- app/components/Commander/CommanderV2.tsx app/components/Search/SearchPage.tsx app/components/Search/commanderSearch.ts app/components/Commander/commanderSearch.test.ts0 errors, 6 warnings,
    the same six pre-existing react-hooks/exhaustive-deps warnings the author
    reported. My changes add none.
  • corepack pnpm run typecheck:ratchet138 errors, down from baseline 143,
    identical to the author's number, so my edits introduce no new type errors.

Control-plane fix validation is recorded separately in lopu-workflow-fix.md
(YAML parse + step/if assertions, workflow-control-plane-contract.mjs --self-test, bash -n, and the ownership step executed against live repository
data for three cases).

Verdict

The feature work is well-built and the risky-looking type change is actually a
correction. The three Commander defects I fixed are all selection/identity
issues created by inserting a new positional tier into an index-addressed
dropdown — the natural hazard of this design. The failing check is a control
plane race, not a problem with this PR, and is fixed separately.

View Lopu workflow run

@github-actions
github-actions Bot temporarily deployed to develop-pr-427 August 27, 2026 09:27 Destroyed
@lopugit
lopugit merged commit fc573f0 into develop Aug 27, 2026
66 checks passed
@github-actions github-actions Bot mentioned this pull request Aug 27, 2026
@lopugit

lopugit commented Aug 27, 2026

Copy link
Copy Markdown
Owner Author

🤖 Promotion conflict resolution was queued automatically for promote/pr-427-search-commander-overhaul--to-main at exact base main (ae012e21a4218f99f8419c670338b267160bd657).

Conflicted source paths: TESTING.md, remix/CHANGELOG.md.

The trusted worker will reconstruct, verify, publish, and attest the review branch; no manual branch update is needed.

@github-actions

Copy link
Copy Markdown
Contributor

🤖 Lopu is resolving the promotion conflict for promote/pr-427-search-commander-overhaul--to-main.

Immutable plan: c38fbe7c3c7ee84b7d4bab2e684f5a7b0878d7d079b739a4bc11e8b4748e93b8 · workflow run

lopugit added a commit that referenced this pull request Aug 27, 2026
…overhaul--to-main

[Promote] Overhaul web search and Commander results (#427)
@lopugit
lopugit deleted the codex/search-commander-overhaul branch September 1, 2026 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant