Weekly defect review: 5 fixes across admin UI, API, TS SDK, and browser extension - #39
Weekly defect review: 5 fixes across admin UI, API, TS SDK, and browser extension#39DennisAlund wants to merge 5 commits into
Conversation
setSlugPrimary, disableSlug, enableSlug, and removeSlug matched the caller's slug against stored slugs with a case-sensitive comparison, while getLinkBySlug and the redirect handler normalize to lowercase first. Slugs are always stored lowercase (addCustomSlugToLink lowercases on insert), and SlugParamSchema permits uppercase in the path, so a request against an existing slug with different casing spuriously returned 404. Lowercase the slug argument at the top of each of the four functions, matching the pattern already used elsewhere.
recentLinksWidget's primarySlug() and LinkRepository.primarySlugByIds (used by top-links) both picked the first auto-generated (non-custom) slug, never checking is_primary. Once an owner sets a custom slug as the link's primary via setSlugPrimary, the link list and detail pages correctly show it everywhere, but the dashboard's Recent Links and Most Clicked widgets kept showing the old auto-generated slug, copying the wrong URL to the clipboard. Prefer the slug flagged is_primary in both places, matching the pick already used on the link list/detail pages.
The keys table rendered each key's scope badge as the literal internal
token ("create"/"read"), untranslated, while the New Key modal
translates the same concept via client.scopeCreate/client.scopeRead.
Non-English admin UIs showed English scope names in this one table.
Map each scope token through the existing client.scopeCreate/
client.scopeRead keys before rendering.
request()'s and requestText()'s !res.ok branch read the body via
res.json() directly and folded any failure (JSON parse error or a
connection reset mid-transfer) into the same generic "HTTP {status}"
message. A transport failure while streaming an *error* response body
therefore surfaced as ShrtnrError(status, ...) instead of the
ShrtnrError(0, ...) every other read path reports, unlike the success
path (already split into readBody() + JSON.parse after the 1.1.3 fix)
and unlike the Python and Dart SDKs, which are consistent here.
Route the error branch's body read through the existing readBody()
helper before parsing, mirroring the success path.
handleTest() called testConnection() directly, without requesting host permission the way handleSave() does, and without normalizing the entered URL to its origin. Two user-visible failures resulted: - Host permission is optional and runtime-granted (no host_permissions in the manifest); without it, the extension's fetch is subject to normal CORS enforcement and the server sends no CORS headers on /api/*, so Test always reported a working server as unreachable on first use. - A URL pasted with a path (e.g. copied from the address bar) hit that path instead of the origin Save would actually use, so Test could 404 on a server that works fine once saved. Request the host permission and normalize to origin in handleTest, mirroring handleSave. Adds error.permissionDeniedTest (en/id/sv) since the existing permissionDenied message says "Click Save again", which doesn't fit the Test button.
|
Flag: BigChart axis labels can mislabel granularity, depending on caller
Separately, the final point's label/tooltip is unconditionally "today"/"today (in progress)" ( Both are demonstrable, but the correct fix isn't a one-line change: the component already receives per-point Generated by Claude Code |
|
Flag: Given the exact same wire payload with a missing/null
Each SDK's own test suite asserts its current behavior, so the three are deliberately (if independently) inconsistent today. Per CLAUDE.md's SDK-parity policy, picking the canonical contract (strict-throw vs. lenient-default vs. add TS validation) and applying it to all three is a public API/error-contract decision, not a local fix. Not fixed directly — needs a decision on which behavior is canonical. Generated by Claude Code |
|
Flag: CLAUDE.md / docs/release-automation.md omit the browser-extension release track Both files' release-track tables list four tracks (app, npm, PyPI, pub.dev). In reality there's a fifth, fully independent track: No functional break, just doc drift a developer following CLAUDE.md wouldn't catch. Left as a comment rather than editing CLAUDE.md/docs directly, since that's a process-governance file the developer likely wants to update deliberately. Not fixed directly. Generated by Claude Code |
|
Flag: CREATE TABLE clicks_backup AS
SELECT c.id, s.slug AS slug, c.clicked_at, ...
FROM clicks c
JOIN slugs s ON s.id = c.slug_id;Any Not fixed directly: this migration has already been applied, and editing an already-applied migration isn't the right fix. Flagging so a developer can decide whether to accept the residual risk as negligible or add a row-count-verification convention for future migrations that recreate FK-cascade-dependent tables. Generated by Claude Code |
|
Flag: on:
check_suite:
types: [completed]
jobs:
migrate:
if: >
github.event.check_suite.conclusion == 'success' &&
github.event.check_suite.head_branch == 'main' &&
github.event.check_suite.app.slug != 'github-actions'Every workflow in this repo runs under the GitHub Actions integration, and check runs from Actions workflows are grouped into a check suite belonging to the I can't verify this against live GitHub Actions behavior from this sandbox, and the blast radius (migrations silently never auto-applying) is high enough that I didn't want to guess. If confirmed, the likely fix is switching the trigger to Generated by Claude Code |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
shrtnr | 5990dd8 | Aug 14 2026, 07:43 PM |
There was a problem hiding this comment.
Pull request overview
This PR fixes five confirmed defects spanning the core service layer, admin dashboard UI, admin i18n, the TypeScript SDK HTTP client, and the browser extension, with new regression tests covering each reported failure mode.
Changes:
- Normalize slug mutation endpoints to match stored lowercase slugs, preventing casing-related 404s.
- Fix dashboard widgets and repository helpers to prefer
is_primaryslugs when selecting a display slug. - Align i18n behavior across the admin keys page, SDK error handling, and browser extension “Test connection” flow (permission request + origin normalization).
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/link-management.ts | Lowercases slug arguments for mutation endpoints to match canonical storage. |
| src/pages/keys.tsx | Translates API key scope badge labels instead of rendering raw tokens. |
| src/db/link-repository.ts | Updates batched display-slug selection ordering to prefer is_primary. |
| src/admin/widgets/dashboard/recent-links.tsx | Picks is_primary slug in Recent Links widget, with fallbacks. |
| src/tests/service/ownership.test.ts | Adds regression tests for case-insensitive slug matching in mutations. |
| src/tests/page/keys-page.test.ts | Adds regression tests for translated/localized scope badges. |
| src/tests/admin/widgets/dashboard/top-links.test.tsx | Adds regression test ensuring Most Clicked uses primary custom slug. |
| src/tests/admin/widgets/dashboard/recent-links.test.tsx | Adds regression test ensuring Recent Links uses primary custom slug. |
| sdk/typescript/tests/client.test.ts | Adds tests for mid-body transport failures on non-2xx responses reporting status 0. |
| sdk/typescript/src/internal/http.ts | Routes non-2xx body reads through readBody() to preserve transport error behavior. |
| browser-extensions/tests/options.test.tsx | Extends options tests to cover host permission request + URL origin normalization for Test. |
| browser-extensions/src/components/ConfigForm.tsx | Fixes Test connection by requesting host permission and normalizing URL to origin. |
| browser-extensions/src/i18n/en.ts | Adds error.permissionDeniedTest (and should also include invalid-URL text key). |
| browser-extensions/src/i18n/id.ts | Adds error.permissionDeniedTest (and should also include invalid-URL text key). |
| browser-extensions/src/i18n/sv.ts | Adds error.permissionDeniedTest (and should also include invalid-URL text key). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| setTestState({ | ||
| kind: "error", | ||
| messageKey: "error.validation", | ||
| params: { message: "Invalid URL" }, | ||
| }); |
| "error.validation": "{message}", | ||
| "error.clipboard": "Copy failed. Select the link above to copy it.", | ||
| "error.permissionDenied": "shrtnr needs permission to talk to {host}. Click Save again and accept.", | ||
| "error.permissionDeniedTest": "shrtnr needs permission to talk to {host}. Click Test again and accept.", |
| "error.validation": "{message}", | ||
| "error.clipboard": "Penyalinan gagal. Pilih tautan di atas untuk menyalinnya.", | ||
| "error.permissionDenied": "shrtnr memerlukan izin untuk berkomunikasi dengan {host}. Klik Simpan lagi dan setujui.", | ||
| "error.permissionDeniedTest": "shrtnr memerlukan izin untuk berkomunikasi dengan {host}. Klik Uji lagi dan setujui.", |
| "error.validation": "{message}", | ||
| "error.clipboard": "Kopiering misslyckades. Markera länken ovan för att kopiera den.", | ||
| "error.permissionDenied": "shrtnr behöver tillstånd att kommunicera med {host}. Klicka på Spara igen och godkänn.", | ||
| "error.permissionDeniedTest": "shrtnr behöver tillstånd att kommunicera med {host}. Klicka på Testa igen och godkänn.", |
Weekly defect-hunting review of the app, API, and SDKs. Five confirmed defects fixed, each with a regression test that fails before the fix and passes after. Several additional findings needed a design/product judgment call and are flagged in PR comments instead of fixed.
Fixes
1. Case-sensitive slug lookup in slug mutation endpoints (
src/services/link-management.ts)setSlugPrimary,disableSlug,enableSlug, andremoveSlugmatched the caller's slug against stored slugs with===, whilegetLinkBySlugand the redirect handler normalize to lowercase first. Slugs are always stored lowercase, and the path schema permits uppercase.Custom-Slugvs storedcustom-slug) spuriously returned 404.src/__tests__/service/ownership.test.ts— 4 new cases, one per function.2. Dashboard widgets ignoring a link's
is_primaryslug (src/admin/widgets/dashboard/recent-links.tsx,src/db/link-repository.ts)recentLinksWidget's slug picker andLinkRepository.primarySlugByIds(used by Most Clicked) both picked the first auto-generated slug, never checkingis_primary.is_primaryin both places.recent-links.test.tsx,top-links.test.tsx— new cases setting a custom slug as primary.3. Untranslated API key scope badges (
src/pages/keys.tsx)"create"/"read"), while the New Key modal translates the same concept.client.scopeCreate/client.scopeReadkeys.keys-page.test.ts— English and Indonesian-locale assertions.4. TypeScript SDK swallowing transport failures on non-2xx responses (
sdk/typescript/src/internal/http.ts)request()/requestText()'s error branch read the body viares.json()directly instead of thereadBody()helper the success path already uses (added in 1.1.3 for this exact class of bug).ShrtnrError(status, "HTTP {status}")instead ofShrtnrError(0, ...), unlike the success path and unlike Python/Dart, which are consistent here.readBody()before parsing.client.test.ts— mid-body stream failure on a 500 response, for bothrequest()andrequestText().5. Broken "Test connection" in the browser extension (
browser-extensions/src/components/ConfigForm.tsx)handleTest()calledtestConnection()directly, without requesting host permission (unlikehandleSave()) and without normalizing the entered URL to its origin./api/*, so clicking "Test connection" against a correctly configured, reachable server on first use always reported it as unreachable. Separately, a URL pasted with a path (e.g. copied from the address bar) tested against the wrong path instead of the originSavewould actually use.handleTest, mirroringhandleSave. Addserror.permissionDeniedTest(en/id/sv) since the existingpermissionDeniedmessage says "Click Save again", which doesn't fit the Test button.options.test.tsx— permission request, URL normalization, and permission-denied cases.Verification
SlugRepository.disablealready prevents this state from ever occurring) and a "raw range code shown untranslated" claim inlinks.tsx(an existing test explicitly asserts the raw code is the intended display, e.g.Clicks (7d)).Flagged for developer judgment (see PR comments)
Bundle.accentcross-SDK parity gap (Python throws, Dart defaults, TS silently undefined)migrations/0004_slug_text_pk.sqluses an INNER JOIN with no row-count verification.github/workflows/migrate.yml's trigger condition may make the workflow permanently deadGenerated by Claude Code