Skip to content

feat: supports host when forwarding to permanent link and provide it as an option in drop down.#4441

Merged
amir20 merged 2 commits intomasterfrom
better-show-logic
Feb 15, 2026
Merged

feat: supports host when forwarding to permanent link and provide it as an option in drop down.#4441
amir20 merged 2 commits intomasterfrom
better-show-logic

Conversation

@amir20
Copy link
Owner

@amir20 amir20 commented Feb 15, 2026

Fixes #4442

@claude
Copy link

claude bot commented Feb 15, 2026

Critical issues:

  • Typo in show.vue:23 - "Expection" should be "Expecting"
  • Equality operator - show.vue:14 uses loose equality (==) instead of strict equality (===)

Suggested improvements:

  • copyPermalink error handling - ContainerActionsToolbar.vue:194-213 doesn't handle copy failure
  • Menu reordering - Search moved above Clear, but no user-facing impact mentioned in PR description
  • Download placement - Moved after separator, may affect muscle memory for existing users

File references:

  • assets/pages/show.vue:23
  • assets/pages/show.vue:14
  • assets/components/ContainerViewer/ContainerActionsToolbar.vue:194

@claude
Copy link

claude bot commented Feb 15, 2026

Critical Issues:

  • Typo in error message (show.vue:23): "Expection" → "Expecting"
  • Potential redirect loop (show.vue:8-27): Watch triggers on every container state change, even when not navigating. Should use { once: true } or check if already on target route.

Code Quality:

  • Menu reordering improves UX (search before clear)
  • copyPermalink() correctly uses router.resolve + window.location.origin
  • Host filtering in show.vue properly handles multi-host scenarios by sorting by startedAt

Concerns:

  • Missing host param validation: copyPermalink() always includes host, but show.vue only filters by host if provided - asymmetric behavior
  • No tests: New permalink feature and updated forwarding logic lack test coverage

Suggested Fix (show.vue:8):

watch(containers, (newValue) => {
  // ... existing logic
}, { once: true });

@amir20 amir20 changed the title feat: adds better logic for show and forward with a helper in menu feat: supports host when forwarding to permanent link and provide it as an option in drop down. Feb 15, 2026
@amir20 amir20 enabled auto-merge (squash) February 15, 2026 15:13
@claude
Copy link

claude bot commented Feb 15, 2026

Review

Critical Issues:

  • Typo in error message (show.vue:23): "Expection" → "Expecting"
  • Race condition potential (show.vue:8-27): Watcher fires on any container change. If user navigates away while containers update, could redirect unexpectedly. Consider using { once: true } or check route validity.
  • Host filtering logic (show.vue:14): Using == for string comparison is fine, but filtering assumes exact host match. If host parameter malformed or URL-encoded differently, match fails silently.

Observations:

  • Menu reordering (ContainerActionsToolbar.vue): Search moved above Clear, Download/Permalink moved to separate section. Good UX grouping.
  • Permalink includes host parameter, enabling multi-host disambiguation (show.vue:197)
  • Fallback when clipboard unavailable shows full URL in toast (ContainerActionsToolbar.vue:203-210)
  • Sort by startedAt DESC ensures newest container selected when duplicates exist (show.vue:15)
  • Changed from visibleContainers to containers - verify this doesn't break filtering intent

File References:

  • show.vue:23 (typo)
  • show.vue:8-27 (watch behavior)
  • ContainerActionsToolbar.vue:194-226 (permalink logic)

@amir20 amir20 merged commit d6109e3 into master Feb 15, 2026
12 checks passed
@amir20 amir20 deleted the better-show-logic branch February 15, 2026 15:17
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.

Copy permanent link

1 participant