Skip to content

fix(shared): classify workspace previews by literal filenames - #10311

Merged
shivamhwp merged 3 commits into
pingdotgg:mainfrom
yashranaway:fix/literal-preview-filenames
Oct 3, 2026
Merged

shivamhwp merged 3 commits into
pingdotgg:mainfrom
yashranaway:fix/literal-preview-filenames

Conversation

@yashranaway

@yashranaway yashranaway commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

What Changed

Classify workspace images and browser documents by their literal filename extension, matching the existing video viewer behavior. Authored Markdown and media URLs retain their separate URL-aware parser.

The shared fix covers server asset validation and image search, web and desktop file viewers, mobile file previews, and provider image-read activity.

Why

Workspace previews strip literal # and ? characters as URL suffixes. This rejects files such as assets#archive/icon#v2.png and accepts non-image filenames such as image.png#notes.txt.

Testing

  • File preview and video tests pass, 39/39; the new classification regressions fail on main.
  • Asset access tests pass, 28/28, including issuing and resolving real workspace URLs while preserving exact image access.
  • Shared package typecheck, targeted lint and formatting, and git diff --check pass.
  • No UI layout changes or browser testing.

Checklist

  • One focused change
  • Explained what changed and why
  • Added regression coverage for the changed behavior
  • No UI layout or animation changes

Model: GPT-6 Astra
Harness: T3 code

Note

Add regression test for workspace asset previews with literal # and ? filenames

Adds an effect-based test in AssetAccess.test.ts that creates workspace files with literal # or ? in their path and verifies issued asset URLs resolve correctly. Also checks rejection of sibling files and filenames with image-like prefixes followed by non-image suffixes.

Macroscope summarized f008404.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Sep 6, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at f008404

Macroscope's review found this PR approvable — This small, well-tested fix corrects preview classification for literal filenames and folders containing # or ?, without changing schemas, security-sensitive code, or deployment behavior. Existing non-preview suffix cases remain explicitly rejected.

No code changes detected at 188e553. Prior analysis still applies.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Sep 13, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
docs/internals/effect-services.md — configured

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: c461410c-782b-40fe-9c99-a8177d626e10
📥 Commits

Reviewing files that changed from the base of the PR and between 188e553 and 10ede2a.

📒 Files selected for processing (2)
  • apps/server/src/assets/AssetAccess.test.ts
  • docs/user/composer.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • docs/user/composer.md

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The preview extension check now evaluates the complete lowercased path. Tests cover preview paths and asset resolution when filenames or directories contain literal # or ? characters. The composer documentation describes extension-based file recognition.

Changes

Workspace preview paths

Layer / File(s) Summary
Preview extension matching
packages/shared/src/filePreview.ts
hasPreviewExtension now checks the complete lowercased path instead of removing query strings and fragments first.
Preview path validation
packages/shared/src/filePreview.test.ts, apps/server/src/assets/AssetAccess.test.ts, docs/user/composer.md
Tests cover recognized and rejected preview paths and asset resolution for paths containing literal # or ? characters. The composer documentation describes extension-based recognition for these paths.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 10ede

The preview change has no established merge-blocking impact; ACP raw output remains available when a suffixed file URL is not promoted.

Architecture Summary

Architecture risk: 🔵 Low · up to 10ede

The change affects 3 systems.

Changed systems: packages/shared, apps/server, docs

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — packages/shared (library) was modified; 2 changed files map to changed impact.
  • observed — apps/server (service) was modified; 1 changed file maps to changed impact.
  • observed — docs (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in packages/shared/src/filePreview.test.ts: The recognizes browser preview path %s test case list drops document.pdf?download=1 and adds document#draft.pdf and reports?old/document.pdf.
  • observed — Modified behavior in packages/shared/src/filePreview.test.ts: Image preview path cases change vector.svg#mark to vector#mark.svg and add photo?edited.JPEG and images#archive/icon.png, while texture.webp and image.avif remain.
  • observed — Modified behavior in packages/shared/src/filePreview.test.ts: The rejects non-preview path %s test retains README.md, src/index.ts, image.png.ts, and png, and adds image.png#notes.txt, image.svg?notes.txt, document.pdf?download=1, report.html#notes.txt, and image%2Epng as additional rejected paths, restructuring the case list into a single-line it.each array.
  • observed — Modified behavior in packages/shared/src/filePreview.ts: hasPreviewExtension now lowercases the whole path and tests the extension against it, whereas before it first stripped any ? query or # fragment via split(/[?#]/, 1); URLs or file paths containing a query or fragment no longer match the preview extensions, affecting isWorkspaceBrowserPreviewPath and isWorkspaceImagePreviewPath.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes classifying workspace previews by literal filenames.
Description check ✅ Passed The description explains the problem, change, and verification. It does not include the template’s scope and approval details, such as a triaged issue or maintainer approval, or why this focused fix n…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 3 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 1, 2026 — with ChatGPT Codex Connector
@shivamhwp

Copy link
Copy Markdown
Collaborator

Tested on two real Android phones (POCO F4 on Android 14, POCO M3 Pro on Android 13) with the dev client against an isolated server, comparing main with this PR. Each file was opened from an @ mention in the new-thread composer.

assets#archive/icon#v2.png, a real PNG. Main cuts the path at #, treats the file as text, and shows "File unavailable". With this PR it renders the image.

icon#v2.png: main vs PR on Android 14 and 13

image.png#notes.txt, a text file. Main sees image.png and fails with "Image unavailable: unknown image format". With this PR it shows the text.

image.png#notes.txt: main vs PR on Android 14 and 13

I updated the branch with main. filePreview.test.ts and AssetAccess.test.ts pass on the updated head (80/80).

One gap main already has: the mobile Files screen still strips ? and # before the audio check, at apps/mobile/src/features/files/filePath.ts:107. Not blocking, and it can be a follow-up.

@shivamhwp
shivamhwp merged commit f391794 into pingdotgg:main Oct 3, 2026
29 checks passed
github-actions Bot added a commit to omarcresp/t3code-flake that referenced this pull request Oct 3, 2026
## What's Changed
* fix(server): runs no longer get stuck by @t3dotgg in pingdotgg/t3code#15048
* fix(usage): Codex Fast and Ultrafast now cost what they bill by @t3dotgg in pingdotgg/t3code#15101
* fix(clients): a dev server left running no longer says the thread is waiting by @t3dotgg in pingdotgg/t3code#15114
* fix(web): a thread that left a shell running shows its unseen completion by @Mnigos in pingdotgg/t3code#14910
* fix(web): mod+enter starts a new thread in the background again by @t3dotgg in pingdotgg/t3code#15060
* feat(usage): show cost by token type, speed, and model detail by @t3dotgg in pingdotgg/t3code#15108
* feat(server): agents can watch a PR and get woken when checks, reviews, or conflicts need them by @t3dotgg in pingdotgg/t3code#15057
* fix(server): keep delegated review rounds on the task API by @t3dotgg in pingdotgg/t3code#15115
* fix(shared): classify workspace previews by literal filenames by @yashranaway in pingdotgg/t3code#10311


**Full Changelog**: pingdotgg/t3code@v0.0.46-nightly.20261003.2623...v0.0.46-nightly.20261003.2632

Upstream release: https://github.com/pingdotgg/t3code/releases/tag/v0.0.46-nightly.20261003.2632
github-actions Bot added a commit to davidvanderklay/t3code-flake that referenced this pull request Oct 3, 2026
## What's Changed
* fix(server): runs no longer get stuck by @t3dotgg in pingdotgg/t3code#15048
* fix(usage): Codex Fast and Ultrafast now cost what they bill by @t3dotgg in pingdotgg/t3code#15101
* fix(clients): a dev server left running no longer says the thread is waiting by @t3dotgg in pingdotgg/t3code#15114
* fix(web): a thread that left a shell running shows its unseen completion by @Mnigos in pingdotgg/t3code#14910
* fix(web): mod+enter starts a new thread in the background again by @t3dotgg in pingdotgg/t3code#15060
* feat(usage): show cost by token type, speed, and model detail by @t3dotgg in pingdotgg/t3code#15108
* feat(server): agents can watch a PR and get woken when checks, reviews, or conflicts need them by @t3dotgg in pingdotgg/t3code#15057
* fix(server): keep delegated review rounds on the task API by @t3dotgg in pingdotgg/t3code#15115
* fix(shared): classify workspace previews by literal filenames by @yashranaway in pingdotgg/t3code#10311


**Full Changelog**: pingdotgg/t3code@v0.0.46-nightly.20261003.2623...v0.0.46-nightly.20261003.2632

Upstream release: https://github.com/pingdotgg/t3code/releases/tag/v0.0.46-nightly.20261003.2632
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:XS 0-9 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants