fix: code review findings — QR size, URL normalization, enableLink, VALID_RANGES, SDK type/sentinel bugs#3
Merged
Conversation
…e() in enableLink, deduplicate VALID_RANGES
- qr.ts: size query param was parsed from the route schema but never read
from the request or forwarded to renderQrSvg; now extracted and passed
- link-management: updateLink called normalizeUrl on create but not on
update, so trailing slashes and bare question marks survived round-trips
- link-management: enableLink used LinkRepository.update({expires_at:null})
instead of the purpose-built LinkRepository.enable(), adding a redundant
DB read and bypassing enable-specific logic
- analytics.ts, bundles.ts: VALID_RANGES was a hand-written literal that
could diverge from TIMELINE_RANGES in schemas.ts; now derived from it
Regression tests added for QR size forwarding and URL normalization in
updateLink (trailing slash, bare question mark, null label clear).
https://claude.ai/code/session_019UcHThUMEX13UdmykzAeMS
The API schema declares size as an integer. The SDK accepted string, forcing callers to pre-stringify. Changed to number; the method now converts to string internally before appending to the query string. Existing test updated; null-clearing tests added for links.update (label, expiresAt) and bundles.update (description). https://claude.ai/code/session_019UcHThUMEX13UdmykzAeMS
links.update() and bundles.update() used None as both the default and the explicit null signal for optional nullable fields (label, expires_at, description, icon). A caller omitting label got the same wire payload as one explicitly passing label=None, so fields could not be cleared without always sending them. Introduced UNSET sentinel in _base.py (singleton). Optional nullable params default to UNSET; only keys explicitly provided by the caller are included in the request body. Passing None sends the field as JSON null, clearing the value on the server. Sync and async resources updated. Regression tests added for both clients. https://claude.ai/code/session_019UcHThUMEX13UdmykzAeMS
Same issue as the Python SDK: links.update() and bundles.update() could not distinguish an omitted label/expiresAt from an explicit null. Used a private const Object _unset = Object() sentinel (identical() check) so omitted params are excluded from the body and null params serialize as JSON null. Regression tests added for null-clearing (label, description) and for confirming omitted fields are absent from the request body. https://claude.ai/code/session_019UcHThUMEX13UdmykzAeMS
Member
Author
|
Items requiring developer judgment — not changed in this PR
Several repository methods (e.g. Generated by Claude Code |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
shrtnr | 888d18e | May 07 2026, 05:26 AM |
There was a problem hiding this comment.
Pull request overview
Fixes several API/app and multi-SDK inconsistencies around QR sizing, URL normalization, link enable semantics, timeline range constants, and “unset vs explicit null” update semantics.
Changes:
- API/App: normalize updated URLs, honor
sizein QR rendering, use canonical timeline ranges, and useLinkRepository.enable(). - SDKs: TypeScript
links.qr()now acceptssize: number; Python/Dart updates now distinguish omitted fields from explicitnullclears via sentinels. - Tests: adds/extends regression coverage for QR sizing, URL normalization, and null-clearing update behavior across app + SDKs.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/link-management.ts | Normalizes URL on update; uses repository enable() method. |
| src/api/qr.ts | Reads size query param and forwards it into QR SVG rendering. |
| src/api/bundles.ts | Derives VALID_RANGES from shared TIMELINE_RANGES. |
| src/api/analytics.ts | Derives VALID_RANGES from shared TIMELINE_RANGES. |
| src/tests/unit/qr.test.ts | Adds regression tests for renderQrSvg() sizing/defaults. |
| src/tests/service/link-service.test.ts | Adds updateLink normalization + null-clearing tests. |
| sdk/typescript/src/resources/links.ts | Changes qr() size option to number and stringifies internally. |
| sdk/typescript/tests/client.test.ts | Adds tests for null-clearing update bodies and numeric QR size. |
| sdk/python/src/shrtnr/_base.py | Introduces UNSET sentinel to distinguish omitted vs explicit None. |
| sdk/python/src/shrtnr/resources/links.py | Uses UNSET defaults so nullable fields can be explicitly cleared. |
| sdk/python/src/shrtnr/resources/bundles.py | Uses UNSET defaults so nullable fields can be explicitly cleared. |
| sdk/python/tests/test_client.py | Adds sync-client tests for omit-vs-clear sentinel behavior. |
| sdk/python/tests/test_async_client.py | Adds async-client tests for omit-vs-clear sentinel behavior. |
| sdk/dart/lib/src/resources/links.dart | Adds sentinel-based omit-vs-clear behavior for link updates. |
| sdk/dart/lib/src/resources/bundles.dart | Adds sentinel-based omit-vs-clear behavior for bundle updates. |
| sdk/dart/test/client_test.dart | Adds Dart tests for omit-vs-clear update semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The service-layer validateSlugLength() capped at 128 while CustomSlugStringSchema capped at 64 and slug_length capped at 16, so callers could request a custom slug that the schema accepted but the service rejected (or vice versa). One global MAX_SLUG_LENGTH constant in src/constants.ts is now the single source of truth, applied to validateSlugLength, CustomSlugStringSchema, slug_length in CreateLinkBodySchema, and the MCP create_link tool input.
Route schema previously accepted size="0" (regex /^\d+$/), and the handler parsed it into 0, which renderQrSvg silently turned into a 0-by-0 SVG. Schema now coerces to integer and bounds 1..2048, so Hono's validation hook returns 400 on bad input. Handler defensively re-validates size for callers that come in via the admin route (no Zod) and renderQrSvg short-circuits to null on a non-positive or non-integer size as a final safety net. Tests cover 0, -5, abc, 2049 (rejected), 1, 220, 300, 2048 (accepted).
CustomSlugStringSchema max moved 64 -> 128, slug_length max moved 16 -> 128, and QR size moved from a regex string to a coerced integer with min=1/max=2048. None of these change the SDK surface: existing methods still take string slugs and integer sizes, and the SDKs do not enforce server-side bounds locally. Hash bumped to record review per CLAUDE.md spec-hash workflow.
Replaces the Object?-with-sentinel update(int id, {label, expiresAt}) signature
with update(Link link) / update(Bundle bundle). Callers now read a record,
clone-and-edit via copyWith, and pass the modified value back. The omit-vs-clear
ambiguity moves into copyWith, where it is encapsulated in the model class via
a private _Unset sentinel that external callers cannot construct or match.
This restores compile-time type checking on label/expiresAt/description/icon
that the previous Object? widening removed, and aligns with idiomatic Dart
practice (immutable models with copyWith). Bumps Dart SDK to 1.1.0; the
breaking change is intentional and documented in CHANGELOG.md.
The Python and TypeScript SDKs keep their existing partial-update shapes;
each language now implements the same set/clear/omit feature in its own idiom.
The CustomSlugStringSchema parameterized test had a 65-char rejection case pinned to the old 64-char cap. Replace with the new 128/129 boundary so the test continues to enforce "reject anything over the documented maximum" against the unified MAX_SLUG_LENGTH = 128 constant.
Comment on lines
+14
to
+21
| let size: number | undefined; | ||
| if (sizeParam !== null) { | ||
| const parsed = Number(sizeParam); | ||
| if (!Number.isFinite(parsed) || !Number.isInteger(parsed) || parsed < 1) { | ||
| return json({ error: "size must be a positive integer" }, 400); | ||
| } | ||
| size = parsed; | ||
| } |
Member
Author
There was a problem hiding this comment.
Good catch, missed the admin route in the first pass. Pulled MIN_QR_SIZE/MAX_QR_SIZE/DEFAULT_QR_SIZE into src/constants.ts and wired them through the Zod schema, the handler, and the renderer so the bound is one place. Admin-route 2049 rejection now has its own test alongside the OpenAPI one.
Comment on lines
1
to
+7
| name: shrtnr | ||
| description: Dart client for the shrtnr URL shortener API. Create short links, manage custom slugs, and read click analytics. | ||
| version: 1.0.1 | ||
| version: 1.1.0 | ||
| homepage: https://oddb.it/shrtnr-website-pub | ||
| repository: https://github.com/oddbit/shrtnr | ||
| issue_tracker: https://github.com/oddbit/shrtnr/issues | ||
| # x-spec-hash: sha256:3b6e9163bac619b9bbef7ba774b2cd06a9a968f1223b85ba302a018c4bee3b57 | ||
| # x-spec-hash: sha256:0d54031654375b3a269cd23d05d66ff44a4b10a894d09b55ed2c1c66405c25b8 |
Member
Author
There was a problem hiding this comment.
Right, breaking signature change on 1.x should bump to 2.x under SemVer. Bumped to 2.0.0 in pubspec.yaml and updated the CHANGELOG heading.
Comment on lines
+259
to
+276
| Link copyWith({ | ||
| int? id, | ||
| String? url, | ||
| Object? label = _unset, | ||
| int? createdAt, | ||
| Object? expiresAt = _unset, | ||
| Object? createdVia = _unset, | ||
| String? createdBy, | ||
| List<Slug>? slugs, | ||
| int? totalClicks, | ||
| Object? deltaPct = _unset, | ||
| }) { | ||
| return Link( | ||
| id: id ?? this.id, | ||
| url: url ?? this.url, | ||
| label: identical(label, _unset) ? this.label : label as String?, | ||
| createdAt: createdAt ?? this.createdAt, | ||
| expiresAt: identical(expiresAt, _unset) ? this.expiresAt : expiresAt as int?, |
Member
Author
There was a problem hiding this comment.
Leaving this. The Object? = _unset shape is the standard Dart idiom for omit-vs-explicit-null in copyWith: it's exactly what freezed generates, and the equivalents (Optional<T>, record-based (T,) wrappers) make every call site noisier even for the common non-null set case (copyWith(label: const Maybe('x')) vs copyWith(label: 'x')).
The model fields themselves stay typed (Link.label is String?). The Object? only leaks at the copyWith parameter, the runtime cast as String? throws on a wrong-type pass, and IDE auto-complete picks the field's declared type. That seems like a fair trade for the call-site ergonomics, and the test suite covers omit/clear/set across both classes.
The "populates KV when a link is created" test triggered the createLink handler's autoLabelLink waitUntil branch, which does a real fetch() with a 5s AbortSignal against the test URL. On CI that fetch hangs long enough to push the next beforeEach past its 10s hookTimeout, failing "populates KV when a custom slug is added" deterministically. Passing a label gates out the autoLabelLink branch and keeps the assertion focused on KV write-through.
The QR size upper bound (2048) was only enforced via the OpenAPI Zod schema on /_/api/links/:id/qr. The admin route /_/admin/api/links/:id/qr goes through the same handler but skips Zod, so a caller could ask for arbitrarily large SVGs and force the renderer to do unbounded work. Hoists MIN_QR_SIZE / MAX_QR_SIZE / DEFAULT_QR_SIZE into src/constants.ts and threads them through: - src/api/qr.ts handler validation (now rejects > MAX_QR_SIZE) - src/api/links.ts Zod schema and OpenAPI description - src/qr.ts renderer min check and default OpenAPI description templated from the same constants so it cannot drift; resulting string is byte-identical so spec hash unchanged. Regression test covers the admin route 400 on size=2049.
The default slug-pick when no ?slug= query param is provided was link.slugs.find(s => s.is_custom) ?? link.slugs[0]. After SlugRepository .setPrimary or after disabling/removing a primary custom slug, the "primary" slug can be the auto-generated one, while a custom slug sits non-primary alongside it. The find(is_custom) pick returned that non-primary custom slug, contradicting the documented "Defaults to the link's primary slug" behavior. Same pattern lived in two places, both fixed: - src/api/qr.ts handleLinkQr default-slug pick - src/mcp/server.ts get_link_qr default-slug pick (also corrects the parameter description from "custom slug or primary" to "primary slug") Regression test: link with auto-slug primary + non-primary custom slug must produce the same QR for /qr and /qr?slug=<auto-slug>.
Under SemVer, a breaking API change on a 1.x package should bump the major. The 1.1.0 entry already documented LinksResource.update and BundlesResource.update as taking a Link/Bundle object instead of an id plus named parameters. Renames the in-progress entry to 2.0.0 and bumps pubspec.yaml accordingly. Spec hash is unchanged so no parity work required.
Comment on lines
206
to
+211
| options: { size?: number; fg?: string; bg?: string } = {}, | ||
| ): string | null { | ||
| if (options.size !== undefined && (!Number.isInteger(options.size) || options.size < MIN_QR_SIZE)) { | ||
| return null; | ||
| } | ||
|
|
Member
Author
There was a problem hiding this comment.
Fair, defense-in-depth on the exported function. Imported MAX_QR_SIZE into src/qr.ts and added > MAX_QR_SIZE to the existing null-return path. Boundary tests cover 2048 (accepted), 2049 and 100000 (null).
Comment on lines
+11
to
+13
| const url = new URL(request.url); | ||
| const requestedSlug = url.searchParams.get("slug") ?? undefined; | ||
| const sizeParam = url.searchParams.get("size"); |
Member
Author
There was a problem hiding this comment.
Confirmed: searchParams.get("slug") returns "" for ?slug=, the ?? undefined only catches null, and the falsy check then silently fell back to the primary. Added an explicit empty-string check that returns 400, matching the OpenAPI route. Test added on the admin route. Broader regex parity (spaces, special chars) is out of scope here.
Comment on lines
171
to
177
| const link = await LinkRepository.getById(env.DB, id); | ||
| if (!link) return fail(404, "Link not found"); | ||
| if (link.created_by !== identity) return fail(403, "Only the link owner can enable this link"); | ||
| const enabled = await LinkRepository.update(env.DB, id, { expires_at: null }); | ||
| const enabled = await LinkRepository.enable(env.DB, id); | ||
|
|
||
| await Promise.all( | ||
| enabled!.slugs.map((s) => |
Member
Author
There was a problem hiding this comment.
Real race. Mapped the null to fail(404, "Link not found") and dropped the ! assertions on the subsequent reads. disableLink has the same shape but pre-dates this PR; flagging for a separate change rather than expanding scope here.
| // 1.0 | ||
| await client.links.update(42, label: null); | ||
|
|
||
| // 1.1 |
Member
Author
There was a problem hiding this comment.
Missed when bumping the version heading. Relabeled the example comments as // 1.x and // 2.0.
| /// always sent on the wire; pass a [Link] produced by [Link.copyWith] with | ||
| /// the desired changes (use `null` on a nullable field to clear it). | ||
| /// Read-only fields like [Link.id] are used for routing; server-managed | ||
| /// fields like slugs and click counts are ignored by the server. |
Member
Author
There was a problem hiding this comment.
Right, UpdateLinkBodySchema is .strict() so extras would be rejected, not ignored. Reworded to say the SDK doesn't include those fields in the payload.
| /// [Bundle.accent]) are always sent on the wire; pass a [Bundle] produced | ||
| /// by [Bundle.copyWith] with the desired changes (use `null` on a nullable | ||
| /// field to clear it). [BundleWithSummary] is also accepted because it | ||
| /// extends [Bundle]; its summary fields are ignored by the server. |
Member
Author
There was a problem hiding this comment.
Same issue, same fix. Reworded to clarify the SDK leaves summary fields out of the body so they never reach the strict server schema.
renderQrSvg already validated MIN_QR_SIZE but not the upper bound, relying entirely on the HTTP-facing handler to cap size. The function is exported and called from src/api/qr.ts and src/mcp/server.ts; if a new caller forgets to pre-validate, an oversized size silently passes through to QR generation and produces an arbitrarily large SVG. Adds the > MAX_QR_SIZE branch to the existing nullable-return path. Unit tests cover 2049 and 100000 (rejected) and 2048 (accepted at the boundary).
The OpenAPI route rejects empty slugs via Zod regex
/^[a-zA-Z0-9_-]+$/. The admin route bypasses Zod and goes straight to
handleLinkQr, where searchParams.get("slug") returns "" for ?slug=.
"" is falsy, so the handler silently fell back to the primary slug
default and returned a QR for a slug the caller didn't ask for. That
is inconsistent with the OpenAPI route's 400 and surprising on its
own merits.
Adds an explicit empty-string check that mirrors the OpenAPI behavior.
Other malformed slugs (spaces, special chars) still pass through to
the slug lookup and return 404 if not found; matching Zod's full regex
on the admin route is out of scope.
LinkRepository.enable() returns null if the link no longer exists when the existence check fires (i.e., a concurrent delete won the race between the service's getById and the repository's UPDATE). enableLink asserted non-null with `!` and would crash on `enabled!.slugs`, surfacing as a 500. Maps the null return to fail(404, "Link not found") and removes the non-null assertions on the subsequent reads, matching how the rest of the service handles "link not found" responses. disableLink has the same shape; left for a separate change rather than expanding this PR's scope.
- CHANGELOG: example comments said "// 1.1" but the release is now 2.0.0 after the SemVer bump; relabel as "// 1.x" and "// 2.0". - Resource docstrings on update(): replace "ignored by the server" with language that reflects the actual behavior. UpdateLinkBody and UpdateBundleBody schemas are .strict() server-side and would reject unknown fields; the SDK simply doesn't include them in the payload.
DennisAlund
pushed a commit
that referenced
this pull request
May 7, 2026
Conflict was textual only: this branch added TIMELINE_RANGES to src/constants.ts; main's #3 added MIN/MAX/DEFAULT_QR_SIZE in the same spot. Kept both. Also re-pointed analytics.ts and bundles.ts (added by #3) at ../constants for TIMELINE_RANGES, since it no longer lives in api/schemas, and consolidated the auto-merged duplicate ../constants imports in api/schemas.ts and mcp/server.ts.
This was referenced May 7, 2026
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
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.
Findings and fixes
App / API
QR size param silently ignored (
src/api/qr.ts)The
sizequery parameter was declared in the route schema but never read from the request or forwarded torenderQrSvg(). Every QR code was rendered at the hardcoded default (220 px) regardless of what the caller passed.URL normalization missing in
updateLink(src/services/link-management.ts)createLinkcallednormalizeUrl()before storage;updateLinkdid not. Trailing slashes and bare?survived update round-trips, producing inconsistent stored URLs.enableLinkused wrong repository method (src/services/link-management.ts)enableLinkcalledLinkRepository.update(db, id, { expires_at: null })instead of the purpose-builtLinkRepository.enable(). This triggered an extra DB read and bypassed any enable-specific logic in the repository.VALID_RANGESduplicated (src/api/analytics.ts,src/api/bundles.ts)Both files hand-wrote the same range set that already exists as
TIMELINE_RANGESinschemas.ts. Now derived from the canonical source.TypeScript SDK
qr()size parameter typed asstring(sdk/typescript/src/resources/links.ts)The API schema declares
sizeas an integer. The SDK acceptedstring, requiring callers to pre-stringify. Changed tonumber; the method converts internally before appending to the query string.Python SDK
update()cannot clear nullable fields (sdk/python/src/shrtnr/resources/)links.update()andbundles.update()usedNoneas both the default and the explicit null signal forlabel,expires_at,description, andicon. Omitting a field was indistinguishable from clearing it. Introduced anUNSETsingleton sentinel in_base.py; nullable params default toUNSETand are excluded from the request body unless explicitly provided.Dart SDK
Same
update()sentinel issue (sdk/dart/lib/src/resources/)Applied the equivalent fix using a private
const Object _unset = Object()sentinel withidentical()checks.Tests
Regression tests added for every fix:
src/__tests__/unit/qr.test.ts:renderQrSvgrespects size, uses 220 defaultsrc/__tests__/service/link-service.test.ts:updateLinkstrips trailing slash, strips bare?, clears label with nullsdk/typescript/tests/client.test.ts: null-clearing forlinks.update(label, expiresAt) andbundles.update(description); size type correctionsdk/python/tests/test_client.pyandtest_async_client.py: sentinel behavior for both clientssdk/dart/test/client_test.dart: null-clearing and omit-field assertions for links and bundleshttps://claude.ai/code/session_019UcHThUMEX13UdmykzAeMS
Generated by Claude Code